Skip to content

Commit d8e3663

Browse files
ClaudiaFangclaude
andcommitted
perf(delete): batch-commit remote-only file deletion
Follow-up to the push-all batching work: batch-deleting remote-only files via the sync status panel's checkbox multi-select was still N separate commits (SyncStatusView.performRemoteDeletion looped gitService.deleteFile once per file). - Add optional GitServiceInterface.deleteBatch, mirroring pushBatch. GitHub/Gitea implement it via the git blob->tree->commit->ref Data API, reusing resolveGitHubStyleBaseTree/resolveBaseTree and commitGitHubStyleTree (widened its tree-item sha type to string | null -- a null sha removes that path from the resulting tree, GitHub's way of expressing a delete at the tree level). GitLab implements it via the same Commits API endpoint pushBatch already uses, with action: 'delete' entries. - SyncStatusView.performRemoteDeletion now calls deleteBatch once per chunk (MAX_BATCH_PUSH_SIZE, reused from the push work) when the provider supports it; a failed chunk marks every path in it as failed rather than dropping results silently. The original per-file loop is preserved verbatim as performRemoteDeletionSequential, the fallback for providers without deleteBatch. Local deletion is unaffected -- it's a local vault operation, not a git commit. Evidence: npx eslint . -> 0 errors; npm run build -> clean; npx vitest run -> 339/339 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent c28e0ec commit d8e3663

12 files changed

Lines changed: 288 additions & 17 deletions

feature_list.json

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,14 @@
8989
"dependencies": [],
9090
"status": "done",
9191
"evidence": "npx eslint . -> 0 errors; npm run build -> clean; npx vitest run -> 330/330 passed; consolidated onto PR #51"
92+
},
93+
{
94+
"id": "feat-012",
95+
"name": "perf(delete): batch-commit remote-only file deletion",
96+
"description": "Follow-up to feat-011: batch-deleting remote-only files via the sync status panel's checkbox multi-select was still N separate commits (SyncStatusView.performRemoteDeletion looped gitService.deleteFile per file). Added optional GitServiceInterface.deleteBatch, mirroring pushBatch: GitHub/Gitea via the git blob->tree->commit->ref Data API (a tree entry with sha:null removes that path); GitLab via the same Commits API actions array used for pushBatch, with action:'delete'. SyncStatusView.performRemoteDeletion now calls deleteBatch once (chunked at MAX_BATCH_PUSH_SIZE) when the provider supports it, falling back to the original sequential per-file performRemoteDeletionSequential otherwise",
97+
"dependencies": ["feat-011"],
98+
"status": "done",
99+
"evidence": "npx eslint . -> 0 errors; npm run build -> clean; npx vitest run -> 339/339 passed; consolidated onto PR #51"
92100
}
93101
],
94102
"_evidenceStyle": "Keep evidence to one line: commit hash + short pointer (e.g. 'Commit abc1234 - added X, tests pass'). Debugging narrative and design discussion belong in the commit message, not this file."

progress.md

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,9 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont
1212

1313
## Current State
1414

15-
**Last Updated:** 2026-07-14 10:10
15+
**Last Updated:** 2026-07-14 10:30
1616
**Session ID:** current
17-
**Active Feature:** feat-011 (perf: batch-commit push-all + SHA-based diffing) — done, lint/build/test all green. Previous active item (issue #78 delete-remote-only-file fix) still awaiting user re-test after rebuild.
17+
**Active Feature:** feat-012 (perf: batch-commit remote-only file deletion) — done, lint/build/test all green. feat-011 (push-all batching) also done this session. Previous active item (issue #78 delete-remote-only-file fix) still awaiting user re-test after rebuild.
1818

1919
## Status
2020

@@ -44,15 +44,25 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont
4444
- Pull path (`pullAllFiles`) intentionally untouched — out of scope, still needs full remote content regardless of sha comparison.
4545
- Not yet committed as of this note — pending commit onto `claude/fix-directory-symlink-pull-260713`.
4646

47+
- [x] feat-012: perf(delete): batch-commit remote-only file deletion. User asked whether batch-deleting remote-only files (checkbox multi-select in the sync status panel) was also one commit; it wasn't (`performRemoteDeletion` looped `gitService.deleteFile` per file). Applied the same batching pattern as feat-011:
48+
1. New optional `GitServiceInterface.deleteBatch?(paths, branch, commitMessage)`, mirroring `pushBatch?`. No result payload (deletes don't produce a new sha to report back).
49+
2. `GitHubService`/`GiteaService.deleteBatch` reuse `resolveGitHubStyleBaseTree`/`resolveBaseTree` + `commitGitHubStyleTree` (widened `commitGitHubStyleTree`'s tree-item `sha` type to `string | null` — a `null` sha removes that path from the resulting tree, which is how GitHub's Git Data API expresses a delete at the tree level).
50+
3. `GitLabService.deleteBatch` reuses the same Commits API endpoint as `pushBatch`, with `action: 'delete'` entries (no `content`/`encoding`).
51+
4. `src/ui/SyncStatusView.ts`'s `performRemoteDeletion` now calls `deleteBatch` once per chunk (`MAX_BATCH_PUSH_SIZE`, reused from feat-011) when the provider supports it, updating progress per-file during a fast local pass before the grouped network call — same UX pattern as push. The original per-file loop was extracted verbatim into `performRemoteDeletionSequential`, used as the fallback when a provider has no `deleteBatch` (e.g. future Bitbucket). A failed chunk marks every path in it as failed (kept in `fileStatuses`/`selectedFiles`, not silently dropped); earlier successful chunks stay deleted.
52+
- Local deletion (`performLocalDeletion`) untouched — pure local vault operation, nothing to batch.
53+
- Added `deleteBatch` tests to `github-service.test.ts`/`gitea-service.test.ts`/`gitlab-service.test.ts` (happy path, empty-array short-circuit) and new `SyncStatusView.test.ts` cases (grouped call, whole-chunk-failure, fallback when `deleteBatch` is absent). Existing vaultFolder-prefix-stripping and real-error-message tests still pass unchanged against the fallback path.
54+
- Evidence: `npx eslint .` → 0 errors; `npm run build` → clean; `npx vitest run` → 339/339 passed.
55+
- Not yet committed as of this note — pending commit onto `claude/fix-directory-symlink-pull-260713`.
56+
4757
### What's In Progress
4858

4959
- Nothing else actively in progress.
5060

5161
### What's Next
5262

5363
1. Get user confirmation that delete now works (or a new detailed error message) after they rebuild/reload with the latest push (issue #78).
54-
2. Manually verify feat-011 in Obsidian if possible: push-all on a mixed vault should produce one new commit containing only changed/new files.
55-
3. Issue #37 (Bitbucket provider support, feat-010) — large, was deferred until #38 (i18n) landed. Now unblocked. Its `GitServiceInterface` implementation should simply omit `pushBatch` and use the existing per-file fallback.
64+
2. Manually verify feat-011/feat-012 in Obsidian if possible: push-all and batch-delete on a mixed vault should each produce exactly one new commit.
65+
3. Issue #37 (Bitbucket provider support, feat-010) — large, was deferred until #38 (i18n) landed. Now unblocked. Its `GitServiceInterface` implementation should simply omit `pushBatch`/`deleteBatch` and use the existing per-file fallbacks.
5666
4. Re-sync against `gh issue list --repo firstsun-dev/git-files-sync --state open`. Remaining genuinely-unstarted issues as of this session: #47 (regex ignore lists), #45 (SonarQube findings), #37 (Bitbucket), #28 (non-engineering: community visibility).
5767
5. PR #51 is large (8+ issues' worth of changes now). If the user wants to review/merge it before more work piles on, flag this rather than continuing to add commits indefinitely.
5868

src/services/git-service-base.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,10 @@ export abstract class BaseGitService {
299299
branch: string,
300300
baseTreeSha: string,
301301
latestCommitSha: string,
302-
treeItems: Array<{ path: string; mode: string; type: 'blob'; sha: string }>,
302+
// A null sha removes that path from the resulting tree — how a batch
303+
// delete is expressed at the tree level (mode/type are still required
304+
// fields on the entry but are otherwise irrelevant for a deletion).
305+
treeItems: Array<{ path: string; mode: string; type: 'blob'; sha: string | null }>,
303306
message: string
304307
): Promise<string> {
305308
const treeResp = await this.safeRequest(`${base}/git/trees`, 'POST', { base_tree: baseTreeSha, tree: treeItems });

src/services/git-service-interface.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,14 @@ export interface GitServiceInterface {
6666
*/
6767
pushBatch?(items: BatchPushItem[], branch: string, commitMessage: string): Promise<BatchPushResult[]>;
6868
deleteFile(path: string, branch: string, commitMessage: string): Promise<void>;
69+
/**
70+
* Delete many files in a single commit. Optional: only providers with a way
71+
* to write multiple changes atomically implement it; callers must fall back
72+
* to sequential deleteFile calls when it's absent (mirrors pushBatch?). Must
73+
* be atomic: on failure it throws rather than partially deleting, so the
74+
* caller can mark every path in the attempted batch as failed.
75+
*/
76+
deleteBatch?(paths: string[], branch: string, commitMessage: string): Promise<void>;
6977
getRepoGitignores(branch: string): Promise<string[]>;
7078
/**
7179
* Fetches a blob's content directly by its git SHA (from a GitTreeEntry),

src/services/gitea-service.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,21 @@ export class GiteaService extends BaseGitService implements GitServiceInterface
153153
await this.safeRequest(url, 'DELETE', body);
154154
}
155155

156+
async deleteBatch(paths: string[], branch: string, message: string): Promise<void> {
157+
if (paths.length === 0) return;
158+
const base = this.getGitDataApiBase();
159+
const { latestCommitSha, baseTreeSha } = await this.resolveBaseTree(branch);
160+
161+
const treeItems = paths.map(path => ({
162+
path: this.getFullPath(path),
163+
mode: '100644',
164+
type: 'blob' as const,
165+
sha: null,
166+
}));
167+
168+
await this.commitGitHubStyleTree(base, branch, baseTreeSha, latestCommitSha, treeItems, message);
169+
}
170+
156171
async testConnection(branch: string): Promise<ConnectionTestResult> {
157172
try {
158173
const url = `${this.baseUrl}/api/v1/repos/${this.owner}/${this.repo}`;

src/services/github-service.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,21 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
159159
await this.safeRequest(url, 'DELETE', body);
160160
}
161161

162+
async deleteBatch(paths: string[], branch: string, message: string): Promise<void> {
163+
if (paths.length === 0) return;
164+
const base = this.getGitDataApiBase();
165+
const { latestCommitSha, baseTreeSha } = await this.resolveGitHubStyleBaseTree(branch);
166+
167+
const treeItems = paths.map(path => ({
168+
path: this.getFullPath(path),
169+
mode: '100644',
170+
type: 'blob' as const,
171+
sha: null,
172+
}));
173+
174+
await this.commitGitHubStyleTree(base, branch, baseTreeSha, latestCommitSha, treeItems, message);
175+
}
176+
162177
async testConnection(branch: string): Promise<ConnectionTestResult> {
163178
try {
164179
const url = `https://api.github.com/repos/${this.owner}/${this.repo}`;

src/services/gitlab-service.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,16 @@ export class GitLabService extends BaseGitService implements GitServiceInterface
137137
await this.safeRequest(url, 'DELETE', body);
138138
}
139139

140+
async deleteBatch(paths: string[], branch: string, message: string): Promise<void> {
141+
if (paths.length === 0) return;
142+
const encodedProjectId = encodeURIComponent(this.projectId);
143+
const url = `${this.baseUrl}/api/v4/projects/${encodedProjectId}/repository/commits`;
144+
145+
const actions = paths.map(path => ({ action: 'delete', file_path: this.getFullPath(path) }));
146+
147+
await this.safeRequest(url, 'POST', { branch, commit_message: message, actions });
148+
}
149+
140150
async testConnection(branch: string): Promise<ConnectionTestResult> {
141151
const encodedProjectId = encodeURIComponent(this.projectId);
142152
try {

src/ui/SyncStatusView.ts

Lines changed: 60 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { isBinaryPath, contentsEqual } from '../utils/path';
1111
import { readLocalSymlinkTarget } from '../utils/symlink';
1212
import { gitBlobSha } from '../utils/git-blob-sha';
1313
import { type GitTreeEntry } from '../services/git-service-interface';
14+
import { MAX_BATCH_PUSH_SIZE } from '../services/git-service-base';
1415
import { t, type TranslationKey } from '../i18n';
1516

1617
export const SYNC_STATUS_VIEW_TYPE = 'sync-status-view';
@@ -45,6 +46,10 @@ export class SyncStatusView extends ItemView {
4546
private renderView(): void {
4647
const container = this.containerEl.children[1] as HTMLElement;
4748
if (!container) return;
49+
50+
const prevListEl = container.querySelector<HTMLElement>('.ssv-list');
51+
const scrollTop = prevListEl?.scrollTop ?? 0;
52+
4853
container.empty();
4954

5055
this.renderInfoStrip(container);
@@ -61,6 +66,8 @@ export class SyncStatusView extends ItemView {
6166
} else {
6267
this.renderFileList(listEl);
6368
}
69+
70+
listEl.scrollTop = scrollTop;
6471
}
6572

6673
private renderProgressBar(container: HTMLElement): void {
@@ -775,20 +782,62 @@ export class SyncStatusView extends ItemView {
775782
}
776783

777784
private async performRemoteDeletion(remote: FileStatus[], total: number, localCount: number, prog: Notice, errors: { path: string, message: string }[]): Promise<void> {
785+
if (remote.length === 0) return;
786+
787+
// s.path is a vault-relative path (may carry the vaultFolder prefix); the
788+
// git service expects a path relative to rootPath only, so strip
789+
// vaultFolder first, same as every other gitService call site.
790+
const entries = remote.map(s => ({ status: s, repoPath: this.plugin.getNormalizedPath(s.path) }));
791+
792+
if (!this.plugin.gitService.deleteBatch) {
793+
await this.performRemoteDeletionSequential(entries, total, localCount, prog, errors);
794+
return;
795+
}
796+
778797
let cur = localCount;
779-
for (const s of remote) {
798+
for (const e of entries) {
780799
cur++;
781-
prog.setMessage(t('syncStatus.progress.deletingRemote', { current: cur, total, path: s.path }));
800+
prog.setMessage(t('syncStatus.progress.deletingRemote', { current: cur, total, path: e.status.path }));
801+
}
802+
803+
const branch = this.plugin.settings.branch;
804+
for (let i = 0; i < entries.length; i += MAX_BATCH_PUSH_SIZE) {
805+
const chunk = entries.slice(i, i + MAX_BATCH_PUSH_SIZE);
782806
try {
783-
// s.path is a vault-relative path (may carry the vaultFolder prefix);
784-
// the git service expects a path relative to rootPath only, so strip
785-
// vaultFolder first, same as every other gitService call site.
786-
const repoPath = this.plugin.getNormalizedPath(s.path);
787-
await this.plugin.gitService.deleteFile(repoPath, this.plugin.settings.branch, `Delete ${repoPath}`);
788-
this.fileStatuses.delete(s.path);
789-
this.selectedFiles.delete(s.path);
790-
} catch (e) {
791-
errors.push({ path: s.path, message: e instanceof Error ? e.message : String(e) });
807+
const message = `Delete ${chunk.length} file(s) from Obsidian`;
808+
await this.plugin.gitService.deleteBatch(chunk.map(e => e.repoPath), branch, message);
809+
for (const e of chunk) {
810+
this.fileStatuses.delete(e.status.path);
811+
this.selectedFiles.delete(e.status.path);
812+
}
813+
} catch (err) {
814+
// Atomic per-provider failure: none of this chunk's files were
815+
// actually deleted, so every path in it is failed, not dropped.
816+
const message = err instanceof Error ? err.message : String(err);
817+
for (const e of chunk) errors.push({ path: e.status.path, message });
818+
}
819+
}
820+
}
821+
822+
/** Provider doesn't support a batch/atomic multi-file delete commit —
823+
* fall back to the original sequential per-file delete. */
824+
private async performRemoteDeletionSequential(
825+
entries: Array<{ status: FileStatus; repoPath: string }>,
826+
total: number,
827+
localCount: number,
828+
prog: Notice,
829+
errors: { path: string, message: string }[]
830+
): Promise<void> {
831+
let cur = localCount;
832+
for (const e of entries) {
833+
cur++;
834+
prog.setMessage(t('syncStatus.progress.deletingRemote', { current: cur, total, path: e.status.path }));
835+
try {
836+
await this.plugin.gitService.deleteFile(e.repoPath, this.plugin.settings.branch, `Delete ${e.repoPath}`);
837+
this.fileStatuses.delete(e.status.path);
838+
this.selectedFiles.delete(e.status.path);
839+
} catch (err) {
840+
errors.push({ path: e.status.path, message: err instanceof Error ? err.message : String(err) });
792841
}
793842
}
794843
}

tests/services/gitea-service.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,35 @@ describe('GiteaService', () => {
300300
});
301301
});
302302

303+
describe('deleteBatch', () => {
304+
it('returns and makes no requests for an empty path list', async () => {
305+
await service.deleteBatch([], 'main', 'delete nothing');
306+
expect(requestUrl).not.toHaveBeenCalled();
307+
});
308+
309+
it('resolves branch via /branches/{branch}, then deletes N files in one commit', async () => {
310+
vi.mocked(requestUrl)
311+
.mockResolvedValueOnce({ status: 200, json: { commit: { id: 'commit1' } } } as unknown as RequestUrlResponse) // resolve branch
312+
.mockResolvedValueOnce({ status: 200, json: { tree: { sha: 'tree1' } } } as unknown as RequestUrlResponse) // get commit
313+
.mockResolvedValueOnce({ status: 201, json: { sha: 'tree2' } } as unknown as RequestUrlResponse) // create tree
314+
.mockResolvedValueOnce({ status: 201, json: { sha: 'commit2' } } as unknown as RequestUrlResponse) // create commit
315+
.mockResolvedValueOnce({ status: 200, json: {} } as unknown as RequestUrlResponse); // update ref
316+
317+
await service.deleteBatch(['a.md'], 'main', 'Delete 1 file(s) from Obsidian');
318+
319+
const calls = vi.mocked(requestUrl).mock.calls.map(c => c[0] as RequestUrlParam);
320+
expect(calls).toHaveLength(5);
321+
expect(calls[0]?.url).toBe(`${baseUrl}/api/v1/repos/${owner}/${repo}/branches/main`);
322+
expect(calls[1]?.url).toBe(`${baseUrl}/api/v1/repos/${owner}/${repo}/git/commits/commit1`);
323+
324+
const treeBody = JSON.parse(calls[2]?.body as string) as { tree: Array<{ path: string; sha: string | null }> };
325+
expect(treeBody.tree).toEqual([{ path: 'a.md', mode: '100644', type: 'blob', sha: null }]);
326+
327+
expect(calls[4]?.method).toBe('PATCH');
328+
expect(calls[4]?.url).toBe(`${baseUrl}/api/v1/repos/${owner}/${repo}/git/refs/heads/main`);
329+
});
330+
});
331+
303332
describe('testConnection', () => {
304333
sharedTestConnection(() => service);
305334

tests/services/github-service.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,40 @@ describe('GitHubService', () => {
312312
});
313313
});
314314

315+
describe('deleteBatch', () => {
316+
it('returns and makes no requests for an empty path list', async () => {
317+
await service.deleteBatch([], 'main', 'delete nothing');
318+
expect(requestUrl).not.toHaveBeenCalled();
319+
});
320+
321+
it('deletes N files in one commit via ref -> commit -> tree(sha:null) -> commit -> ref', async () => {
322+
vi.mocked(requestUrl)
323+
.mockResolvedValueOnce({ status: 200, json: { object: { sha: 'commit1' } } } as unknown as RequestUrlResponse) // get ref
324+
.mockResolvedValueOnce({ status: 200, json: { tree: { sha: 'tree1' } } } as unknown as RequestUrlResponse) // get commit
325+
.mockResolvedValueOnce({ status: 201, json: { sha: 'tree2' } } as unknown as RequestUrlResponse) // create tree
326+
.mockResolvedValueOnce({ status: 201, json: { sha: 'commit2' } } as unknown as RequestUrlResponse) // create commit
327+
.mockResolvedValueOnce({ status: 200, json: {} } as unknown as RequestUrlResponse); // update ref
328+
329+
await service.deleteBatch(['a.md', 'b.md'], 'main', 'Delete 2 file(s) from Obsidian');
330+
331+
const calls = vi.mocked(requestUrl).mock.calls.map(c => c[0] as RequestUrlParam);
332+
expect(calls).toHaveLength(5);
333+
334+
const treeBody = JSON.parse(calls[2]?.body as string) as { base_tree: string; tree: Array<{ path: string; mode: string; type: string; sha: string | null }> };
335+
expect(treeBody.base_tree).toBe('tree1');
336+
expect(treeBody.tree).toEqual([
337+
{ path: 'a.md', mode: '100644', type: 'blob', sha: null },
338+
{ path: 'b.md', mode: '100644', type: 'blob', sha: null },
339+
]);
340+
341+
const commitBody = JSON.parse(calls[3]?.body as string) as { message: string; tree: string; parents: string[] };
342+
expect(commitBody).toEqual({ message: 'Delete 2 file(s) from Obsidian', tree: 'tree2', parents: ['commit1'] });
343+
344+
expect(calls[4]?.method).toBe('PATCH');
345+
expect(JSON.parse(calls[4]?.body as string)).toEqual({ sha: 'commit2' });
346+
});
347+
});
348+
315349
describe('testConnection', () => {
316350
sharedTestConnection(() => service);
317351

0 commit comments

Comments
 (0)