diff --git a/docs/api-reference.md b/docs/api-reference.md index 81a39396..22132a09 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -704,6 +704,8 @@ normal `caseName`/`mode`/etc. body) `GET /api/sessions/:id/git-status` is what the bottom-bar Git indicator and its panel read (Settings → Header & Panels → Bottom bar, per-device, default off). It reports what the session's workspace has not committed or pushed. **Read-only and offline:** it never fetches, pulls, commits or writes (it runs `git status` with `--no-optional-locks`, so it does not even refresh the index), which is why `behind` is as of the last `git fetch`. The session is resolved like every session route (ownership via `findSessionOrFail`; another user's session is `404`). +`GET /api/sessions/:id/git-diff?repo=&path=&kind=staged|unstaged|untracked|conflicted` returns the unified diff of one file the panel lists (`{ diff, truncated, binary }`; staged is index vs HEAD, unstaged is working tree vs index, untracked is the whole file as additions). It is what opens when you click a file in the Git panel. `repo` and `path` are matched against the current status rather than trusted, so anything the status does not list is `404`. Read-only (`--no-ext-diff --no-textconv`, so repository config never runs a program), capped at 400 KB, and refused (`400`) for remote and Docker sessions. + **Which repositories.** git finds a repository by walking *up* from the session's working directory, so: - Inside a repository (or at its root): that one repository, whole (a subfolder reports its enclosing repo, `path` says where it is, e.g. `../..`). A nested repo below it is just an untracked folder to the outer one and is not scanned; start the session inside it to see it. diff --git a/src/git-workspace-status.ts b/src/git-workspace-status.ts index add38bea..c0b41f0f 100644 --- a/src/git-workspace-status.ts +++ b/src/git-workspace-status.ts @@ -533,3 +533,58 @@ export async function getGitWorkspaceOverview( if (!repos.length) return emptyOverview('not-a-repo'); return { state: 'ok', repos, reposTruncated: found.truncated, checkedAt: Date.now() }; } + +// ── Per-file diff ────────────────────────────────────────────────────────── + +/** Longest diff handed to the browser; beyond this it is cut at a line boundary and flagged. */ +export const MAX_DIFF_BYTES = 400 * 1024; + +export interface GitFileDiff { + /** Unified diff text (empty when git reports no textual change, e.g. a mode-only edit shows its header). */ + diff: string; + truncated: boolean; + binary: boolean; +} + +/** A repo-relative path git reported, minus anything that could be read as an option or escape the repo. */ +export function isSafeRepoRelativePath(p: string): boolean { + if (!p || p.length > 4096 || p.includes('\0') || p.startsWith('-') || p.startsWith('/')) return false; + return !p.split('/').includes('..'); +} + +/** + * The diff of one changed file, as the panel's rows describe it: `staged` is index vs HEAD, + * `unstaged`/`conflicted` is working tree vs index (a conflict shows git's combined diff), and + * `untracked` is the whole file as additions. Read-only, and `--no-ext-diff --no-textconv` keep a + * repository's own config from running programs on behalf of a click. + */ +export async function getGitFileDiff( + repoRoot: string, + file: { path: string; origPath?: string; kind: GitFileKind }, + opts: { git?: GitRunner } = {} +): Promise { + if (!isSafeRepoRelativePath(file.path) || (file.origPath && !isSafeRepoRelativePath(file.origPath))) { + throw new Error('Invalid path'); + } + const git = opts.git ?? runGit; + const base = ['diff', '--no-color', '--no-ext-diff', '--no-textconv', '-U3']; + let args: string[]; + if (file.kind === 'untracked') args = [...base, '--no-index', '--', '/dev/null', file.path]; + else { + const paths = file.origPath ? [file.origPath, file.path] : [file.path]; + args = file.kind === 'staged' ? [...base, '--cached', '-M', '--', ...paths] : [...base, '--', ...paths]; + } + let out: string; + try { + out = await git(repoRoot, args); + } catch (err) { + // `--no-index` exits 1 when the files differ, which is the normal case for it. + const e = err as { code?: number; stdout?: unknown }; + if (file.kind === 'untracked' && e.code === 1 && typeof e.stdout === 'string') out = e.stdout; + else throw err; + } + const binary = /^Binary files .* differ$/m.test(out) || /^GIT binary patch$/m.test(out); + if (out.length <= MAX_DIFF_BYTES) return { diff: out, truncated: false, binary }; + const cut = out.lastIndexOf('\n', MAX_DIFF_BYTES); + return { diff: out.slice(0, cut > 0 ? cut : MAX_DIFF_BYTES), truncated: true, binary }; +} diff --git a/src/web/public/git-status-ui.js b/src/web/public/git-status-ui.js index d4872f6c..1af96de1 100644 --- a/src/web/public/git-status-ui.js +++ b/src/web/public/git-status-ui.js @@ -214,6 +214,7 @@ Object.assign(CodemanApp.prototype, { }, closeGitStatusPanel() { + this._gitDiffView = null; const panel = this.$('gitStatusPanel'); if (panel) { panel.classList.remove('visible'); @@ -277,6 +278,15 @@ Object.assign(CodemanApp.prototype, { if (!body) return; const overview = this._currentGitStatus(); const el = (tag, cls, text) => this._gitEl(tag, cls, text); + const view = this._gitDiffView; + if (view && view.sessionId === this.activeSessionId) { + // A file's diff is on screen: the 15 s poll re-renders the panel, and must not throw it away. + this._renderGitDiffView(body, view); + if (head) head.textContent = ''; + if (foot) foot.textContent = ''; + return; + } + this._gitDiffView = null; body.replaceChildren(); const clearChrome = () => { if (head) head.textContent = ''; @@ -453,13 +463,13 @@ Object.assign(CodemanApp.prototype, { row.append(name); if (f.origPath) row.append(el('span', 'git-status-orig', `← ${f.origPath}`)); - // Deleted files and untracked folders have nothing to preview. - const previewable = letter !== 'D' && !f.path.endsWith('/') && data.repoRoot; - if (previewable) { + // An untracked folder has no single diff; every other row opens its changes. + if (!f.path.endsWith('/') && data.repoRoot) { row.classList.add('git-status-file--clickable'); row.tabIndex = 0; row.setAttribute('role', 'button'); - const open = () => this.openFilePreview?.(`${data.repoRoot}/${f.path}`, this.activeSessionId); + row.title = 'Show what changed'; + const open = () => this.openGitDiff(data.repoRoot, f, letter); row.addEventListener('click', open); row.addEventListener('keydown', (e) => { if (e.key === 'Enter' || e.key === ' ') { @@ -471,6 +481,94 @@ Object.assign(CodemanApp.prototype, { return row; }, + // ── Diff view ─────────────────────────────────────────────────────────── + + /** Show `file`'s changes in the panel (a Back button returns to the list). */ + async openGitDiff(repoRoot, file, letter) { + const sessionId = this.activeSessionId; + if (!sessionId) return; + const view = { sessionId, repoRoot, file, letter, state: 'loading' }; + this._gitDiffView = view; + this._renderGitStatusPanel(); + const qs = new URLSearchParams({ repo: repoRoot, path: file.path, kind: file.kind }); + const res = await this._api(`/api/sessions/${encodeURIComponent(sessionId)}/git-diff?${qs}`); + // Back, another file or another session while this was in flight: drop the answer. + if (this._gitDiffView !== view) return; + let body = null; + try { + body = res ? await res.json() : null; + } catch { + /* fall through */ + } + if (this._gitDiffView !== view) return; + if (res && res.ok && body?.success) { + view.state = 'ok'; + view.result = body.data; + } else { + view.state = 'error'; + view.error = body?.error || 'Could not read the diff.'; + } + this._renderGitStatusPanel(); + }, + + closeGitDiff() { + this._gitDiffView = null; + this._renderGitStatusPanel(); + }, + + _renderGitDiffView(body, view) { + const el = (tag, cls, text) => this._gitEl(tag, cls, text); + body.replaceChildren(); + const bar = el('div', 'git-diff-bar'); + const back = el('button', 'btn-toolbar btn-sm', '← Back'); + back.type = 'button'; + back.addEventListener('click', () => this.closeGitDiff()); + bar.append(back); + bar.append(el('span', 'git-diff-path', view.file.path)); + const kindLabel = { staged: 'staged', unstaged: 'not staged', untracked: 'new file', conflicted: 'conflict' }; + bar.append(el('span', 'git-diff-kind', kindLabel[view.file.kind] || '')); + if (view.letter !== 'D') { + const open = el('button', 'btn-toolbar btn-sm', 'Open file'); + open.type = 'button'; + open.addEventListener('click', () => + this.openFilePreview?.(`${view.repoRoot}/${view.file.path}`, this.activeSessionId) + ); + bar.append(open); + } + body.append(bar); + + if (view.state === 'loading') { + body.append(el('div', 'git-status-empty', 'Reading the diff…')); + return; + } + if (view.state === 'error') { + body.append(el('div', 'git-status-empty', view.error)); + return; + } + const { diff, truncated, binary } = view.result; + if (binary) body.append(el('div', 'git-status-note', 'This is a binary file; there is no text diff to show.')); + if (!diff.trim()) { + if (!binary) body.append(el('div', 'git-status-empty', 'No textual changes (the file may differ only in mode).')); + return; + } + const pre = el('pre', 'git-diff'); + const frag = document.createDocumentFragment(); + for (const line of diff.split('\n')) { + let cls = 'git-diff-line'; + if (line.startsWith('@@')) cls += ' git-diff-line--hunk'; + else if ( + /^(diff --git|index |--- |\+\+\+ |new file|deleted file|similarity|rename |old mode|new mode)/.test(line) + ) + cls += ' git-diff-line--meta'; + else if (line.startsWith('+')) cls += ' git-diff-line--add'; + else if (line.startsWith('-')) cls += ' git-diff-line--del'; + frag.append(el('span', cls, line + '\n')); + } + pre.append(frag); + body.append(pre); + if (truncated) body.append(el('div', 'git-status-more', 'Diff cut short: it is larger than the viewer shows.')); + }, + _gitCommitRow(c) { const el = (tag, cls, text) => this._gitEl(tag, cls, text); const row = el('div', 'git-status-commit'); diff --git a/src/web/public/styles.css b/src/web/public/styles.css index df51e1de..de6a8db0 100644 --- a/src/web/public/styles.css +++ b/src/web/public/styles.css @@ -19575,3 +19575,62 @@ html .toolbar .btn-git-status.git-status--conflict { .git-status-repo-body { padding: 0.5rem 0.6rem; } + +/* Git status panel: per-file diff view */ +.git-diff-bar { + display: flex; + align-items: center; + gap: 0.45rem; + margin-bottom: 0.4rem; + min-width: 0; +} + +.git-diff-path { + flex: 1; + min-width: 0; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + font-family: 'SF Mono', Monaco, monospace; + font-size: 0.7rem; +} + +.git-diff-kind { + font-size: 0.64rem; + color: var(--text-muted); + white-space: nowrap; +} + +.git-diff { + margin: 0; + padding: 0.3rem 0; + overflow-x: auto; + font-family: 'SF Mono', Monaco, monospace; + font-size: 0.68rem; + line-height: 1.45; + border: 1px solid var(--border); + border-radius: 4px; +} + +.git-diff-line { + display: block; + padding: 0 0.5rem; + white-space: pre; +} + +.git-diff-line--add { + background: color-mix(in srgb, var(--green) 18%, transparent); +} + +.git-diff-line--del { + background: color-mix(in srgb, var(--red) 18%, transparent); +} + +.git-diff-line--hunk { + color: var(--text-muted); + background: var(--bg-hover); +} + +.git-diff-line--meta { + color: var(--text-muted); +} diff --git a/src/web/routes/git-status-routes.ts b/src/web/routes/git-status-routes.ts index 1ba28769..73200aab 100644 --- a/src/web/routes/git-status-routes.ts +++ b/src/web/routes/git-status-routes.ts @@ -11,11 +11,14 @@ */ import type { FastifyInstance } from 'fastify'; -import type { ApiResponse } from '../../types.js'; +import { ApiErrorCode, createErrorResponse, getErrorMessage, type ApiResponse } from '../../types.js'; import { findSessionOrFail } from '../route-helpers.js'; import { emptyOverview, + getGitFileDiff, getGitWorkspaceOverview, + type GitFileDiff, + type GitFileKind, type GitRunner, type GitWorkspaceOverview, } from '../../git-workspace-status.js'; @@ -30,4 +33,30 @@ export function registerGitStatusRoutes(app: FastifyInstance, ctx: SessionPort, if (session.docker) return { success: true, data: emptyOverview('unsupported', { reason: 'docker' }) }; return { success: true, data: await getGitWorkspaceOverview(session.workingDir, { git, fresh: fresh === '1' }) }; }); + + // The diff of one file the panel lists. `repo` and `path` are matched against the CURRENT status + // (a repository this session's folder holds, a path git reported in it) rather than trusted, so + // the route cannot be pointed at an arbitrary directory or file. + app.get('/api/sessions/:id/git-diff', async (req, reply): Promise> => { + const { id } = req.params as { id: string }; + const { repo, path, kind } = req.query as { repo?: string; path?: string; kind?: string }; + const session = findSessionOrFail(ctx, id, req); + if (session.remote || session.docker) { + reply.code(400); + return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Git is not available for remote or Docker sessions'); + } + const overview = await getGitWorkspaceOverview(session.workingDir, { git, fresh: true }); + const status = overview.repos.find((r) => r.status.repoRoot === repo)?.status; + const entry = status?.files.find((f) => f.path === path && f.kind === (kind as GitFileKind)); + if (!status?.repoRoot || !entry) { + reply.code(404); + return createErrorResponse(ApiErrorCode.NOT_FOUND, 'That file has no outstanding change any more'); + } + try { + return { success: true, data: await getGitFileDiff(status.repoRoot, entry, { git }) }; + } catch (err) { + reply.code(500); + return createErrorResponse(ApiErrorCode.INTERNAL_ERROR, `git diff failed: ${getErrorMessage(err)}`); + } + }); } diff --git a/test/git-status.browser.test.ts b/test/git-status.browser.test.ts index e59d360e..0d15bfed 100644 --- a/test/git-status.browser.test.ts +++ b/test/git-status.browser.test.ts @@ -168,21 +168,39 @@ describe('Git status indicator in a real browser', () => { expect(await page.evaluate(() => (window as any).__pwned)).toBeUndefined(); }); - it('clicking a file previews it by its absolute path; a deleted file is not clickable', async () => { + it('clicking a file shows its diff in the panel; Back returns to the list; Open file previews it', async () => { await page.evaluate(() => { (window as any).__previewed = []; (window as any).app.openFilePreview = (p: string) => (window as any).__previewed.push(p); }); await page.click('.git-status-file:has-text("a.txt")'); + await page.waitForSelector('#gitStatusBody .git-diff'); + expect(await page.textContent('#gitStatusBody .git-diff-path')).toBe('a.txt'); + expect(await page.textContent('#gitStatusBody .git-diff-line--del')).toBe('-1\n'); + expect(await page.textContent('#gitStatusBody .git-diff-line--add')).toBe('+2\n'); + // The 15 s poll re-renders the panel; the diff must survive it. + await refresh(); + expect(await page.$('#gitStatusBody .git-diff')).not.toBeNull(); + await page.click('#gitStatusBody button:has-text("Open file")'); expect(await page.evaluate(() => (window as any).__previewed)).toEqual([join(repo, 'a.txt')]); + await page.click('#gitStatusBody button:has-text("Back")'); + await page.waitForSelector('.git-status-file:has-text("a.txt")'); + expect(await page.$('#gitStatusBody .git-diff')).toBeNull(); + }); + + it('an untracked file diffs as all additions, and a deleted file as all removals (with no Open file)', async () => { + await page.click('.git-status-file:has-text("new file.txt")'); + await page.waitForSelector('#gitStatusBody .git-diff-line--add'); + expect(await page.locator('#gitStatusBody .git-diff-line--del').count()).toBe(0); + await page.click('#gitStatusBody button:has-text("Back")'); rmSync(join(repo, 'b.txt')); await refresh(); - await page.waitForFunction(() => - /Deleted|b\.txt/.test(document.getElementById('gitStatusBody')!.textContent ?? '') - ); - const deleted = page.locator('.git-status-file:has(.git-status-badge--D)'); - expect(await deleted.count()).toBe(1); - expect(await deleted.first().getAttribute('role')).toBeNull(); + await page.waitForSelector('.git-status-file:has(.git-status-badge--D)'); + await page.click('.git-status-file:has(.git-status-badge--D)'); + await page.waitForSelector('#gitStatusBody .git-diff-line--del'); + expect(await page.locator('#gitStatusBody .git-diff-line--add').count()).toBe(0); + expect(await page.locator('#gitStatusBody button:has-text("Open file")').count()).toBe(0); + await page.click('#gitStatusBody button:has-text("Back")'); }); it('drags by the header', async () => { diff --git a/test/git-workspace-status.test.ts b/test/git-workspace-status.test.ts index bc0e6ba2..e9b6e617 100644 --- a/test/git-workspace-status.test.ts +++ b/test/git-workspace-status.test.ts @@ -660,3 +660,68 @@ describe('getGitWorkspaceOverview', () => { expect(await isUnrelatedAncestor(home, home, home)).toBe(false); }); }); + +import { MAX_DIFF_BYTES, getGitFileDiff, isSafeRepoRelativePath } from '../src/git-workspace-status.js'; + +describe('isSafeRepoRelativePath', () => { + it.each([ + ['a.txt', true], + ['src/deep/x.ts', true], + ['', false], + ['-rf', false], + ['/etc/passwd', false], + ['../x', false], + ['a/../../x', false], + ['a\0b', false], + ])('%j -> %s', (p, ok) => expect(isSafeRepoRelativePath(p)).toBe(ok)); +}); + +describe('getGitFileDiff', () => { + it('refuses an unsafe path without running git', async () => { + const git = vi.fn(async () => ''); + await expect(getGitFileDiff('/r', { path: '../x', kind: 'unstaged' }, { git })).rejects.toThrow('Invalid path'); + expect(git).not.toHaveBeenCalled(); + }); + + it('builds read-only, option-injection-safe commands per kind', async () => { + const git = vi.fn(async (_cwd: string, _args: string[]) => ''); + await getGitFileDiff('/r', { path: 'a.txt', kind: 'unstaged' }, { git }); + await getGitFileDiff('/r', { path: 'b.txt', origPath: 'old.txt', kind: 'staged' }, { git }); + const [unstaged, staged] = git.mock.calls.map((c) => c[1]); + for (const args of [unstaged, staged]) { + expect(args).toContain('--no-ext-diff'); + expect(args).toContain('--no-textconv'); + expect(args.indexOf('--')).toBeGreaterThan(0); + } + expect(unstaged.slice(-2)).toEqual(['--', 'a.txt']); + expect(staged).toContain('--cached'); + expect(staged.slice(-3)).toEqual(['--', 'old.txt', 'b.txt']); + }); + + it('treats --no-index exit 1 as the normal untracked result', async () => { + const git = vi.fn(async () => { + throw Object.assign(new Error('exit 1'), { code: 1, stdout: '+hello\n' }); + }); + await expect(getGitFileDiff('/r', { path: 'n.txt', kind: 'untracked' }, { git })).resolves.toMatchObject({ + diff: '+hello\n', + }); + const boom = vi.fn(async () => { + throw Object.assign(new Error('exit 128'), { code: 128, stdout: '' }); + }); + await expect(getGitFileDiff('/r', { path: 'n.txt', kind: 'untracked' }, { git: boom })).rejects.toThrow(); + }); + + it('flags binary output and cuts an oversized diff at a line boundary', async () => { + const bin = await getGitFileDiff( + '/r', + { path: 'x.png', kind: 'unstaged' }, + { git: async () => 'Binary files a/x.png and b/x.png differ\n' } + ); + expect(bin.binary).toBe(true); + const big = ('+' + 'x'.repeat(99) + '\n').repeat(Math.ceil(MAX_DIFF_BYTES / 100) + 50); + const cut = await getGitFileDiff('/r', { path: 'big', kind: 'unstaged' }, { git: async () => big }); + expect(cut.truncated).toBe(true); + expect(cut.diff.length).toBeLessThanOrEqual(MAX_DIFF_BYTES); + expect(cut.diff.endsWith('x')).toBe(true); + }); +}); diff --git a/test/routes/git-status-routes.test.ts b/test/routes/git-status-routes.test.ts index ceb6fc98..fb0c7564 100644 --- a/test/routes/git-status-routes.test.ts +++ b/test/routes/git-status-routes.test.ts @@ -4,7 +4,7 @@ * all (remote and Docker sessions). Port: N/A (app.inject()). */ import { execFileSync } from 'node:child_process'; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -153,3 +153,66 @@ describe('GET /api/sessions/:id/git-status', () => { } }); }); + +describe('GET /api/sessions/:id/git-diff', () => { + const url = (q: Record) => `/api/sessions/test-session-1/git-diff?${new URLSearchParams(q)}`; + let root: string; + + beforeEach(() => { + git(dir, 'init', '-q', '-b', 'main'); + writeFileSync(join(dir, 'a.txt'), 'one\n'); + writeFileSync(join(dir, 'b.txt'), 'bee\n'); + git(dir, 'add', '-A'); + git(dir, 'commit', '-q', '-m', 'base'); + writeFileSync(join(dir, 'a.txt'), 'two\n'); + writeFileSync(join(dir, 'b.txt'), 'staged\n'); + git(dir, 'add', 'b.txt'); + writeFileSync(join(dir, 'new.txt'), 'fresh\n'); + root = realpathSync(dir); + }); + + it.each([ + ['unstaged', 'a.txt', ['-one', '+two']], + ['staged', 'b.txt', ['-bee', '+staged']], + ['untracked', 'new.txt', ['+fresh']], + ])('returns the %s diff of %s', async (kind, path, lines) => { + const { app } = await setup(); + const res = await app.inject({ method: 'GET', url: url({ repo: root, path, kind }) }); + expect(res.statusCode).toBe(200); + const { diff, truncated, binary } = res.json().data; + for (const l of lines) expect(diff).toContain(l); + expect(truncated).toBe(false); + expect(binary).toBe(false); + }); + + it('answers 404 for a path or repo the status does not list, running no diff', async () => { + const runner = vi.fn(async () => ''); + const { app } = await setup({ git: runner }); + for (const q of [ + { repo: root, path: '../../etc/passwd', kind: 'unstaged' }, + { repo: '/etc', path: 'a.txt', kind: 'unstaged' }, + { repo: root, path: 'a.txt', kind: 'staged' }, + ]) { + const res = await app.inject({ method: 'GET', url: url(q) }); + expect(res.statusCode).toBe(404); + } + expect(runner.mock.calls.some(([, args]) => args[0] === 'diff')).toBe(false); + }); + + it('does not run git for remote and Docker sessions', async () => { + const runner = vi.fn(async () => ''); + const { app } = await setup({ git: runner }); + session.remote = { host: 'h' }; + const res = await app.inject({ method: 'GET', url: url({ repo: root, path: 'a.txt', kind: 'unstaged' }) }); + expect(res.statusCode).toBe(400); + expect(runner).not.toHaveBeenCalled(); + }); + + it('multi-user: another user’s session is not found', async () => { + process.env.CODEMAN_MULTIUSER = '1'; + const { app } = await setup({ authUser: { username: 'bob', role: 'user' } }); + session.owner = 'alice'; + const res = await app.inject({ method: 'GET', url: url({ repo: root, path: 'a.txt', kind: 'unstaged' }) }); + expect(res.statusCode).toBe(404); + }); +});