obsidian-plugin/firstsun-dev_git-files-sync
1
0
Fork 0
mirror of https://github.com/firstsun-dev/git-files-sync.git synced 2026-07-22 06:54:27 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix(push): avoid stale remote-tree read right after a batch push

Browse source 
After a push completed, SyncStatusView.executeBatchOperation immediately
re-fetched the remote tree via refreshAllStatuses() to update the UI.
GitHub's tree-by-branch-name read can lag a just-completed write by a
moment, so this could misreport a file that was just pushed correctly
as still "modified" (confirmed live: refreshing again a few seconds
later showed it as synced).

SyncManager.pushAllFiles now also returns syncedPaths (path + new sha,
from data already available at the write site: the batch chunk result,
the sequential-push fallback, or the immediate symlink/rename push).
SyncStatusView uses this to mark those files 'synced' directly instead
of re-fetching the remote tree, sidestepping the eventual-consistency
window entirely. Pull is unaffected, still does a full refresh.
This commit is contained in:
ClaudiaFang 2026-07-14 11:53:20 +00:00
parent 114a5759a7
commit 7676325088
4 changed files with 158 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

53
src/logic/sync-manager.ts
View file

@ -14,6 +14,23 @@ type BatchOutcome = 'done' | 'unchanged' | 'conflict';
/** A file classified as needing a push, queued for the grouped batch-commit call. */
type ToPushEntry = { path: string; name: string; repoPath: string; content: string | ArrayBuffer; existingSha?: string };
/**
* Result of a batch push. `syncedPaths` lists every path that's now confirmed
* synced (content just written matches what's now on the remote), with its
* new blob sha when known. The caller uses this to mark those files' UI
* status directly rather than re-fetching the remote tree right after a
* write — GitHub's tree-by-branch-name read can lag a successful write by a
* moment, so an immediate re-fetch can misreport a just-pushed file as
* "modified" even though nothing is actually different.
*/
export type PushResults = {
success: number;
failed: number;
conflicts: number;
errors: Array<{ file: string; error: string }>;
syncedPaths: Array<{ path: string; sha?: string }>;
};
export class SyncManager {
private readonly app: App;
private gitService: GitServiceInterface;
@ -192,7 +209,7 @@ export class SyncManager {
}
}
private async performPush(file: {path: string, name: string}, content: string | ArrayBuffer, existingSha?: string, silent = false) {
private async performPush(file: {path: string, name: string}, content: string | ArrayBuffer, existingSha?: string, silent = false): Promise<string | undefined> {
const repoPath = this.getNormalizedPath(file.path);
const result = await this.gitService.pushFile(
repoPath,
@ -212,6 +229,7 @@ export class SyncManager {
if (newSha) await this.updateMetadata(file.path, newSha);
if (!silent) new Notice(`Pushed ${file.name} to ${this.serviceName}`);
return newSha;
}
/**
@ -360,7 +378,7 @@ export class SyncManager {
files: (TFile | string)[],
onProgress?: (current: number, total: number, fileName: string) => void,
remoteTree?: GitTreeEntry[]
): Promise<{ success: number; failed: number; conflicts: number; errors: Array<{ file: string; error: string }> }> {
): Promise<PushResults> {
return this.processPushBatch(files, onProgress, remoteTree);
}
@ -402,8 +420,8 @@ export class SyncManager {
files: (TFile | string)[],
onProgress?: (current: number, total: number, fileName: string) => void,
remoteTree?: GitTreeEntry[]
): Promise<{ success: number; failed: number; conflicts: number; errors: Array<{ file: string; error: string }> }> {
const results = { success: 0, failed: 0, conflicts: 0, errors: [] as Array<{ file: string; error: string }> };
): Promise<PushResults> {
const results: PushResults = { success: 0, failed: 0, conflicts: 0, errors: [], syncedPaths: [] };
const tree = remoteTree ?? await this.gitService.listFilesDetailed(this.settings.branch, false);
const treeByFullPath = new Map<string, GitTreeEntry>(tree.map(e => [e.path, e]));
@ -418,7 +436,13 @@ export class SyncManager {
try {
const outcome = await this.classifyPushCandidate(fileOrPath, path, name, isString, treeByFullPath, toPush);
if (outcome === 'done') results.success++;
if (outcome === 'done') {
results.success++;
// Symlink/rename pushes are committed immediately outside the
// toPush queue, so the new sha isn't known here — the caller
// still gets to mark the path synced, just without a sha update.
results.syncedPaths.push({ path });
}
else if (outcome === 'conflict') results.conflicts++;
// 'unchanged' and 'queued' don't move any of the counters directly:
// 'unchanged' never did, and 'queued' is resolved by commitPushBatch below.
@ -535,10 +559,7 @@ export class SyncManager {
}
/** Commits every queued file in one or more grouped batch-commit calls. */
private async commitPushBatch(
toPush: ToPushEntry[],
results: { success: number; failed: number; conflicts: number; errors: Array<{ file: string; error: string }> }
): Promise<void> {
private async commitPushBatch(toPush: ToPushEntry[], results: PushResults): Promise<void> {
if (!this.gitService.pushBatch) {
await this.pushSequentialFallback(toPush, results);
return;
@ -551,14 +572,12 @@ export class SyncManager {
/** Provider doesn't support a batch/atomic multi-file commit — fall back to
* the same sequential per-file push used by the single-file flow. */
private async pushSequentialFallback(
toPush: ToPushEntry[],
results: { success: number; failed: number; conflicts: number; errors: Array<{ file: string; error: string }> }
): Promise<void> {
private async pushSequentialFallback(toPush: ToPushEntry[], results: PushResults): Promise<void> {
for (const f of toPush) {
try {
await this.performPush({ path: f.path, name: f.name }, f.content, f.existingSha, true);
const sha = await this.performPush({ path: f.path, name: f.name }, f.content, f.existingSha, true);
results.success++;
results.syncedPaths.push({ path: f.path, sha });
} catch (e) {
results.failed++;
results.errors.push({ file: f.path, error: e instanceof Error ? e.message : String(e) });
@ -566,10 +585,7 @@ export class SyncManager {
}
}
private async commitOneChunk(
chunk: ToPushEntry[],
results: { success: number; failed: number; conflicts: number; errors: Array<{ file: string; error: string }> }
): Promise<void> {
private async commitOneChunk(chunk: ToPushEntry[], results: PushResults): Promise<void> {
try {
const commitMessage = `Push ${chunk.length} file(s) from Obsidian`;
const batchResults = await this.gitService.pushBatch!(
@ -582,6 +598,7 @@ export class SyncManager {
const sha = shaByPath.get(f.repoPath);
if (sha) await this.updateMetadata(f.path, sha);
results.success++;
results.syncedPaths.push({ path: f.path, sha });
}
} catch (e) {
// Atomic per-provider failure: none of this chunk's files were

31
src/ui/SyncStatusView.ts
View file

@ -13,6 +13,7 @@ import { gitBlobSha } from '../utils/git-blob-sha';
import { type GitTreeEntry } from '../services/git-service-interface';
import { MAX_BATCH_PUSH_SIZE } from '../services/git-service-base';
import { t, type TranslationKey } from '../i18n';
import { type PushResults } from '../logic/sync-manager';
export const SYNC_STATUS_VIEW_TYPE = 'sync-status-view';
@ -682,6 +683,24 @@ export class SyncStatusView extends ItemView {
await this.executeBatchOperation(filter, op, files);
}
/**
* Marks just-pushed paths as 'synced' directly from data already in hand
* (the content that was just written, and the new sha when the provider
* reported one), instead of re-fetching the remote tree. Used in place of
* refreshAllStatuses() right after a push — see the call site's comment.
*/
private applyOptimisticSyncedStatus(syncedPaths: Array<{ path: string; sha?: string }>): void {
for (const { path, sha } of syncedPaths) {
const existing = this.fileStatuses.get(path);
this.fileStatuses.set(path, {
...existing,
path,
status: 'synced',
remoteSha: sha ?? existing?.remoteSha,
});
}
}
private async executeBatchOperation(filter: 'modified' | 'selected', op: 'push' | 'pull', files: Array<string | TFile>): Promise<void> {
const runVerb = op === 'push' ? t('main.verb.pushing') : t('main.verb.pulling');
const prog = new Notice(t('main.progress.running', { verb: runVerb, total: files.length }), 0);
@ -695,7 +714,17 @@ export class SyncStatusView extends ItemView {
if (filter === 'selected') this.selectedFiles.clear();
const doneVerb = op === 'push' ? t('main.verb.push') : t('main.verb.pull');
new Notice(t('syncStatus.notice.opCompleted', { verb: doneVerb }));
await this.refreshAllStatuses();
if (op === 'push') {
// Mark just-pushed files synced directly instead of re-fetching the
// remote tree: GitHub's tree-by-branch-name read can lag a few
// seconds behind a just-completed write, so an immediate refresh
// can misreport a file we know just synced correctly as "modified".
this.applyOptimisticSyncedStatus((results as PushResults).syncedPaths);
this.renderView();
} else {
await this.refreshAllStatuses();
}
} catch (e) {
prog.hide();
const failVerb = op === 'push' ? t('main.verb.push') : t('main.verb.pull');

28
tests/logic/sync-manager-batch.test.ts
View file

@ -128,6 +128,33 @@ describe('SyncManager Batch Operations', () => {
{ path: 'a.md', content: 'content-a', existedRemotely: false },
{ path: 'b.md', content: 'content-b', existedRemotely: false },
]);
// syncedPaths lets the caller mark these files synced directly, without
// a follow-up remote read that could race a provider's eventual
// consistency window (see SyncStatusView's use of this field).
expect(results.syncedPaths).toEqual([
{ path: 'a.md', sha: 'sha-a' },
{ path: 'b.md', sha: 'sha-b' },
]);
});
it('reports syncedPaths via the sequential fallback when the provider has no pushBatch', async () => {
const files = ['a.md', 'b.md'];
const adapter = mockApp.vault.adapter as Mocked<DataAdapter>;
vi.mocked(adapter.exists).mockResolvedValue(true);
vi.mocked(adapter.read).mockImplementation(async (p) => (p === 'a.md' ? 'content-a' : 'content-b'));
vi.mocked(mockGitService.listFilesDetailed).mockResolvedValue([]);
mockGitService.pushBatch = undefined;
vi.mocked(mockGitService.pushFile).mockImplementation(async (path) => ({ path, sha: `sha-${path}` }));
const results = await manager.pushAllFiles(files);
expect(results.success).toBe(2);
expect(mockGitService.pushFile).toHaveBeenCalledTimes(2);
expect(results.syncedPaths).toEqual([
{ path: 'a.md', sha: 'sha-a.md' },
{ path: 'b.md', sha: 'sha-b.md' },
]);
});
it('handles a mixed binary + text batch, computing blob shas for both', async () => {
@ -166,6 +193,7 @@ describe('SyncManager Batch Operations', () => {
{ file: 'a.md', error: 'commit failed' },
{ file: 'b.md', error: 'commit failed' },
]);
expect(results.syncedPaths).toEqual([]);
});
it('skips both getFile and pushBatch when the local blob sha already matches the tree', async () => {

65
tests/ui/SyncStatusView.test.ts
View file

@ -209,3 +209,68 @@ describe('SyncStatusView.identifyExtraFiles folder/remote-record collisions', ()
expect(extra).toEqual(['notes/hidden.md']);
});
});
describe('SyncStatusView post-push status update', () => {
beforeAll(() => { setupObsidianDOM(); });
// Regression test: GitHub's tree-by-branch-name read can lag a moment behind
// a just-completed write (GraphQL createCommitOnBranch or otherwise), so
// re-fetching the remote tree immediately after a push can misreport a file
// that was just pushed correctly as still "modified". The fix marks
// successfully-pushed paths 'synced' directly from the push result instead
// of trusting an immediate remote re-read.
it('marks pushed files synced from the push result instead of re-fetching the remote tree', async () => {
const pushAllFiles = vi.fn().mockResolvedValue({
success: 2, failed: 0, conflicts: 0, errors: [],
syncedPaths: [{ path: 'a.md', sha: 'sha-a' }, { path: 'b.md', sha: 'sha-b' }],
});
const plugin = {
settings: { branch: 'main', vaultFolder: '' },
gitService: {},
sync: { pushAllFiles },
} as unknown as GitLabFilesPush;
const app = { vault: { adapter: { exists: vi.fn().mockResolvedValue(false) } } };
const leaf = { app } as unknown as WorkspaceLeaf;
const view = new SyncStatusView(leaf, plugin);
const statuses = (view as unknown as { fileStatuses: Map<string, FileStatus> }).fileStatuses;
statuses.set('a.md', { path: 'a.md', status: 'modified', localContent: '' });
statuses.set('b.md', { path: 'b.md', status: 'modified', localContent: '' });
const refreshSpy = vi.spyOn(view, 'refreshAllStatuses').mockResolvedValue(undefined);
await (view as unknown as {
executeBatchOperation(filter: 'modified' | 'selected', op: 'push' | 'pull', files: Array<string>): Promise<void>
}).executeBatchOperation('modified', 'push', ['a.md', 'b.md']);
expect(pushAllFiles).toHaveBeenCalledTimes(1);
// The fix: no remote tree re-fetch right after push (that read is what
// can lag GitHub's write and misreport the file as still modified).
expect(refreshSpy).not.toHaveBeenCalled();
expect(statuses.get('a.md')).toEqual({ path: 'a.md', status: 'synced', localContent: '', remoteSha: 'sha-a' });
expect(statuses.get('b.md')).toEqual({ path: 'b.md', status: 'synced', localContent: '', remoteSha: 'sha-b' });
});
it('still does a full remote refresh after a pull (unaffected by this fix)', async () => {
const pullAllFiles = vi.fn().mockResolvedValue({ success: 1, failed: 0, conflicts: 0, errors: [] });
const plugin = {
settings: { branch: 'main', vaultFolder: '' },
gitService: {},
sync: { pullAllFiles },
} as unknown as GitLabFilesPush;
const app = { vault: { adapter: { exists: vi.fn().mockResolvedValue(false) } } };
const leaf = { app } as unknown as WorkspaceLeaf;
const view = new SyncStatusView(leaf, plugin);
const refreshSpy = vi.spyOn(view, 'refreshAllStatuses').mockResolvedValue(undefined);
await (view as unknown as {
executeBatchOperation(filter: 'modified' | 'selected', op: 'push' | 'pull', files: Array<string>): Promise<void>
}).executeBatchOperation('modified', 'pull', ['a.md']);
expect(pullAllFiles).toHaveBeenCalledTimes(1);
expect(refreshSpy).toHaveBeenCalledTimes(1);
});
});