From 76763250881e382614d1aa31b1d8ca742cc9b014 Mon Sep 17 00:00:00 2001 From: ClaudiaFang Date: Tue, 14 Jul 2026 11:53:20 +0000 Subject: [PATCH] fix(push): avoid stale remote-tree read right after a batch push 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. --- src/logic/sync-manager.ts | 53 ++++++++++++++------- src/ui/SyncStatusView.ts | 31 +++++++++++- tests/logic/sync-manager-batch.test.ts | 28 +++++++++++ tests/ui/SyncStatusView.test.ts | 65 ++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 19 deletions(-) diff --git a/src/logic/sync-manager.ts b/src/logic/sync-manager.ts index 79c16d6..3d0808d 100644 --- a/src/logic/sync-manager.ts +++ b/src/logic/sync-manager.ts @@ -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 { 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 { 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 { + 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(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 { + private async commitPushBatch(toPush: ToPushEntry[], results: PushResults): Promise { 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 { + private async pushSequentialFallback(toPush: ToPushEntry[], results: PushResults): Promise { 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 { + private async commitOneChunk(chunk: ToPushEntry[], results: PushResults): Promise { 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 diff --git a/src/ui/SyncStatusView.ts b/src/ui/SyncStatusView.ts index ec44e41..6e5119e 100644 --- a/src/ui/SyncStatusView.ts +++ b/src/ui/SyncStatusView.ts @@ -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): Promise { 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'); diff --git a/tests/logic/sync-manager-batch.test.ts b/tests/logic/sync-manager-batch.test.ts index 50d2a21..32309cc 100644 --- a/tests/logic/sync-manager-batch.test.ts +++ b/tests/logic/sync-manager-batch.test.ts @@ -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; + + 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 () => { diff --git a/tests/ui/SyncStatusView.test.ts b/tests/ui/SyncStatusView.test.ts index 14e4b66..27c120d 100644 --- a/tests/ui/SyncStatusView.test.ts +++ b/tests/ui/SyncStatusView.test.ts @@ -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 }).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): Promise + }).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): Promise + }).executeBatchOperation('modified', 'pull', ['a.md']); + + expect(pullAllFiles).toHaveBeenCalledTimes(1); + expect(refreshSpy).toHaveBeenCalledTimes(1); + }); +});