From 0e86e2bf984450b2a92692f9eeee294f3d1a717c Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 04:42:44 +0000 Subject: [PATCH 1/5] refactor(sync): split SyncStatusRefreshService into discovery/resolver/reconciler Give file discovery, status resolution, and rename reconciliation each a single owning class (SyncFileDiscovery, SyncStatusResolver, RenameReconciler) instead of one 661-line service implementing all three algorithms. SyncStatusRefreshService now only orchestrates the three plus the incremental create/modify/delete/rename handlers, per the module boundaries in docs/architecture.md. Co-Authored-By: Claude Sonnet 5 --- src/logic/sync/RenameReconciler.ts | 86 +++ src/logic/sync/SyncFileDiscovery.ts | 192 +++++++ src/logic/sync/SyncStatusRefreshService.ts | 488 ++---------------- src/logic/sync/SyncStatusResolver.ts | 239 +++++++++ tests/logic/sync/RenameReconciler.test.ts | 105 ++++ tests/logic/sync/SyncFileDiscovery.test.ts | 124 +++++ .../sync/SyncStatusRefreshService.test.ts | 46 -- tests/logic/sync/SyncStatusResolver.test.ts | 136 +++++ 8 files changed, 922 insertions(+), 494 deletions(-) create mode 100644 src/logic/sync/RenameReconciler.ts create mode 100644 src/logic/sync/SyncFileDiscovery.ts create mode 100644 src/logic/sync/SyncStatusResolver.ts create mode 100644 tests/logic/sync/RenameReconciler.test.ts create mode 100644 tests/logic/sync/SyncFileDiscovery.test.ts create mode 100644 tests/logic/sync/SyncStatusResolver.test.ts diff --git a/src/logic/sync/RenameReconciler.ts b/src/logic/sync/RenameReconciler.ts new file mode 100644 index 0000000..8f244fa --- /dev/null +++ b/src/logic/sync/RenameReconciler.ts @@ -0,0 +1,86 @@ +import { isSyncMetadataAtPath, type GitLabFilesPushSettings } from '../../settings'; +import type { SyncStatusService } from '../sync-status-service'; +import type { GitTreeEntry } from '../../services/git-service-interface'; +import { gitBlobSha } from '../../utils/git-blob-sha'; +import type { SyncManager } from './SyncManager'; + +export interface RenameReconcilerDependencies { + settings: () => GitLabFilesPushSettings; + syncManager: () => SyncManager; + /** Republishes a single path's status after it's been reconciled as a rename target. */ + refreshFileStatus(path: string, remoteEntry: GitTreeEntry | undefined): Promise; +} + +/** + * Reconciles renames performed outside the plugin (e.g. via git directly): + * matches an orphaned tracked path against an unsynced local file by blob + * sha, and relocates the sync metadata. Deliberately owns no discovery or + * status-resolution logic. + */ +export class RenameReconciler { + constructor( + private readonly dependencies: RenameReconcilerDependencies, + private readonly statuses: SyncStatusService, + ) {} + + async reconcileOutOfBandMoves(remoteMap: Map): Promise { + const orphansBySha = this.orphanedMoveSourcesBySha(remoteMap); + if (orphansBySha.size === 0) return; + const candidatesBySha = await this.unsyncedMoveDestinationsBySha(remoteMap, orphansBySha); + + for (const [sha, orphanPaths] of orphansBySha) { + if (orphanPaths.length !== 1) continue; + const newPaths = candidatesBySha.get(sha); + if (!newPaths || newPaths.length !== 1) continue; + const oldPath = orphanPaths[0] as string; + const newPath = newPaths[0] as string; + await this.dependencies.syncManager().trackRename(newPath, oldPath); + this.statuses.delete(oldPath); + await this.dependencies.refreshFileStatus(newPath, remoteMap.get(newPath)); + } + } + + pendingMoveOldPaths(): Set { + const paths = new Set(); + for (const metadata of Object.values(this.dependencies.settings().syncMetadata ?? {})) { + if (metadata.renamedFrom) paths.add(metadata.renamedFrom); + } + return paths; + } + + private orphanedMoveSourcesBySha(remoteMap: Map): Map { + const metadata = this.dependencies.settings().syncMetadata ?? {}; + const orphansBySha = new Map(); + for (const [path, status] of this.statuses) { + // A tracked-then-deleted file is now classified `local-deleted` + // (not `remote-only`), so both qualify as an orphaned move + // source: the remote entry still exists, sync metadata is + // present for the path, and it isn't itself a pending move. + if (status.status !== 'remote-only' && status.status !== 'local-deleted') continue; + const pathMetadata = metadata[path]; + if (!isSyncMetadataAtPath(pathMetadata, path) || pathMetadata.renamedFrom) continue; + const entry = remoteMap.get(path); + if (!entry || entry.symlink || !entry.sha) continue; + const paths = orphansBySha.get(entry.sha) ?? []; + paths.push(path); + orphansBySha.set(entry.sha, paths); + } + return orphansBySha; + } + + private async unsyncedMoveDestinationsBySha( + remoteMap: Map, + orphansBySha: Map, + ): Promise> { + const candidatesBySha = new Map(); + for (const [path, status] of this.statuses) { + if (status.status !== 'unsynced' || status.localContent === undefined || remoteMap.has(path)) continue; + const sha = await gitBlobSha(status.localContent); + if (!orphansBySha.has(sha)) continue; + const paths = candidatesBySha.get(sha) ?? []; + paths.push(path); + candidatesBySha.set(sha, paths); + } + return candidatesBySha; + } +} diff --git a/src/logic/sync/SyncFileDiscovery.ts b/src/logic/sync/SyncFileDiscovery.ts new file mode 100644 index 0000000..72a8177 --- /dev/null +++ b/src/logic/sync/SyncFileDiscovery.ts @@ -0,0 +1,192 @@ +import { type App, TFile } from 'obsidian'; +import { getEffectiveSymlinkHandling, isSyncMetadataAtPath, type GitLabFilesPushSettings } from '../../settings'; +import type { GitignoreManager } from '../gitignore-manager'; +import type { SyncStatusService } from '../sync-status-service'; +import type { GitServiceInterface, GitTreeEntry } from '../../services/git-service-interface'; +import { readLocalSymlinkTarget } from '../../utils/symlink'; + +export interface SyncFileDiscoveryDependencies { + app: App; + settings: () => GitLabFilesPushSettings; + gitService: () => GitServiceInterface; + gitignoreManager: () => GitignoreManager; + filterFilesByVaultFolder(files: TFile[]): TFile[]; + filterPathByVaultFolder(path: string): boolean; + getNormalizedPath(path: string): string; + getVaultPath(path: string): string; +} + +export interface DiscoveredFiles { + local: TFile[]; + remoteEntries: GitTreeEntry[]; + remoteHead?: string; + remoteMap: Map; + localMap: Set; + allMap: Map; + hiddenLocalPaths: Set; +} + +/** + * Enumerates what exists locally and remotely (vault files, hidden files, + * remote tree, vault-folder scope, .gitignore, symlink filtering) and + * classifies remote-only-vs-local-deleted for paths with no local file. + * Deliberately owns no status-resolution or rename-reconciliation logic. + */ +export class SyncFileDiscovery { + constructor( + private readonly dependencies: SyncFileDiscoveryDependencies, + private readonly statuses: SyncStatusService, + ) {} + + async discoverFiles(): Promise { + const { app } = this.dependencies; + const settings = this.dependencies.settings(); + const gitService = this.dependencies.gitService(); + const gitignoreManager = this.dependencies.gitignoreManager(); + const allFiles = app.vault.getFiles(); + let local = this.dependencies.filterFilesByVaultFolder(allFiles); + const remoteHead = await gitService.getBranchHead?.(settings.branch); + const remoteEntries = await gitService.listFilesDetailed(remoteHead ?? settings.branch, false); + + await gitignoreManager.loadGitignores(remoteEntries); + + const remoteMap = new Map(); + const skipSymlinks = getEffectiveSymlinkHandling(settings) === 'skip'; + for (const entry of remoteEntries) { + if (entry.symlink && skipSymlinks) continue; + const normalized = this.getNormalizedRemotePath(entry.path); + if (normalized === null) continue; + + const vaultPath = this.dependencies.getVaultPath(normalized); + if (!gitignoreManager.isIgnored(normalized)) remoteMap.set(vaultPath, entry); + } + + local = local.filter(file => !gitignoreManager.isIgnored(this.dependencies.getNormalizedPath(file.path))); + const hiddenLocalPaths = await this.discoverHiddenLocalFiles(); + const filteredHiddenPaths = new Set( + hiddenLocalPaths + .filter(path => this.dependencies.filterPathByVaultFolder(path)) + .filter(path => !gitignoreManager.isIgnored(this.dependencies.getNormalizedPath(path))), + ); + + return { + local, + remoteEntries, + remoteHead, + remoteMap, + localMap: new Set([...local.map(file => file.path), ...filteredHiddenPaths]), + allMap: new Map(allFiles.map(file => [file.path, file])), + hiddenLocalPaths: filteredHiddenPaths, + }; + } + + getNormalizedRemotePath(remotePath: string): string | null { + const rootPath = this.dependencies.settings().rootPath; + if (!rootPath) return remotePath; + const cleanRoot = rootPath.endsWith('/') ? rootPath : `${rootPath}/`; + if (remotePath.startsWith(cleanRoot)) return remotePath.substring(cleanRoot.length); + return remotePath === rootPath ? '' : null; + } + + async discoverHiddenLocalFiles(): Promise { + const result: string[] = []; + await this.recursiveScan(this.dependencies.settings().vaultFolder || '', result); + return result; + } + + async recursiveScan(folderPath: string, result: string[]): Promise { + try { + const listing = await this.dependencies.app.vault.adapter.list(folderPath); + for (const file of listing.files) { + if (!this.isHidden(file)) continue; + if (readLocalSymlinkTarget(this.dependencies.app, file) !== null || await this.isLocalFile(file)) result.push(file); + } + for (const folder of listing.folders) { + if (folder === '.git' || folder.endsWith('/.git')) continue; + if (readLocalSymlinkTarget(this.dependencies.app, folder) !== null) { + if (this.isHidden(folder)) result.push(folder); + continue; + } + await this.recursiveScan(folder, result); + } + } catch { + // Some Obsidian adapters do not support raw directory listing. + } + } + + async identifyExtraFiles( + remoteMap: Map, + localFilePaths: Set, + allLocalFileMap: Map, + pendingMoveOldPaths: Set = new Set(), + ): Promise> { + const extra: Array = []; + for (const [vaultPath] of remoteMap) { + if (localFilePaths.has(vaultPath) || pendingMoveOldPaths.has(vaultPath)) continue; + + let localFile = allLocalFileMap.get(vaultPath); + if (!localFile) { + const abstractFile = this.dependencies.app.vault.getAbstractFileByPath(vaultPath); + if (abstractFile instanceof TFile) localFile = abstractFile; + } + + if (localFile) extra.push(localFile); + else if (await this.isLocalFile(vaultPath)) extra.push(vaultPath); + else { + // No local file at all. A tracked file that's since been + // removed locally (sync metadata still present for the path, + // and not a pending move source) is a *local deletion* — a + // potential remote deletion — distinct from a never-tracked + // remote-only file, which is simply available to download. + this.statuses.set(vaultPath, { + path: vaultPath, + status: this.statuses.classify({ + localExists: false, + remoteExists: true, + wasTracked: this.wasTrackedBeforeDelete(vaultPath), + }), + }); + } + } + return extra; + } + + initializeFileStatuses(localFiles: TFile[]): void { + for (const file of localFiles) this.statuses.set(file.path, { file, path: file.path, status: 'checking' }); + } + + getCheckableFiles( + local: TFile[], + extra: Array, + hiddenLocalPaths: Set, + ): Array { + const extraPaths = new Set(extra.map(file => typeof file === 'string' ? file : file.path)); + const hiddenToAdd = [...hiddenLocalPaths].filter(path => !extraPaths.has(path)); + const gitignoreManager = this.dependencies.gitignoreManager(); + return [...local, ...extra, ...hiddenToAdd].filter(file => { + const path = typeof file === 'string' ? file : file.path; + return !gitignoreManager.isIgnored(this.dependencies.getNormalizedPath(path)); + }); + } + + /** + * Whether `vaultPath` was previously tracked locally and has since been + * removed (sync metadata present for the path, and not a pending move + * source). Used to distinguish a `local-deleted` row from a + * never-tracked `remote-only` download candidate. + */ + private wasTrackedBeforeDelete(vaultPath: string): boolean { + const metadata = this.dependencies.settings().syncMetadata; + const pathMetadata = metadata ? metadata[vaultPath] : undefined; + return isSyncMetadataAtPath(pathMetadata, vaultPath) && !pathMetadata.renamedFrom; + } + + private isHidden(path: string): boolean { + return path.split('/').some(part => part.startsWith('.')); + } + + private async isLocalFile(vaultPath: string): Promise { + const stat = await this.dependencies.app.vault.adapter.stat(vaultPath); + return stat?.type === 'file'; + } +} diff --git a/src/logic/sync/SyncStatusRefreshService.ts b/src/logic/sync/SyncStatusRefreshService.ts index e6b5236..ad2078e 100644 --- a/src/logic/sync/SyncStatusRefreshService.ts +++ b/src/logic/sync/SyncStatusRefreshService.ts @@ -1,13 +1,15 @@ import { type App, TFile } from 'obsidian'; -import { getEffectiveSymlinkHandling, isSyncMetadataAtPath, type GitLabFilesPushSettings, type SymlinkHandling } from '../../settings'; +import type { GitLabFilesPushSettings } from '../../settings'; import type { GitignoreManager } from '../gitignore-manager'; import { type FileStatus, SyncStatusService } from '../sync-status-service'; import type { GitServiceInterface, GitTreeEntry } from '../../services/git-service-interface'; import { gitBlobSha } from '../../utils/git-blob-sha'; import { logger } from '../../utils/logger'; -import { contentsEqual, isBinaryPath } from '../../utils/path'; -import { readLocalSymlinkTarget } from '../../utils/symlink'; +import { isBinaryPath } from '../../utils/path'; import type { SyncManager } from './SyncManager'; +import { SyncFileDiscovery } from './SyncFileDiscovery'; +import { SyncStatusResolver } from './SyncStatusResolver'; +import { RenameReconciler } from './RenameReconciler'; export interface SyncStatusRefreshDependencies { app: App; @@ -33,22 +35,17 @@ export interface SyncStatusRefreshResult { remoteEntries: GitTreeEntry[]; } -interface DiscoveredFiles { - local: TFile[]; - remoteEntries: GitTreeEntry[]; - remoteHead?: string; - remoteMap: Map; - localMap: Set; - allMap: Map; - hiddenLocalPaths: Set; -} - /** - * Scans local/remote state and projects it into the shared status store. - * It deliberately exposes no rendering or notification concepts. + * Orchestrates a full status refresh — discovery → resolve → reconcile + * renames → publish — and owns the incremental create/modify/delete/rename + * handlers used between full refreshes. Discovery, status resolution, and + * rename reconciliation each live in their own collaborator; this class + * deliberately exposes no rendering or notification concepts. */ export class SyncStatusRefreshService { - private static readonly STATUS_CHECK_CONCURRENCY = 8; + private readonly discovery: SyncFileDiscovery; + private readonly resolver: SyncStatusResolver; + private readonly renameReconciler: RenameReconciler; /** Per-path monotonic revision ordering async content writes (create read vs a raced modify). */ private readonly contentRevisions = new Map(); @@ -56,26 +53,37 @@ export class SyncStatusRefreshService { constructor( private readonly dependencies: SyncStatusRefreshDependencies, private readonly statuses: SyncStatusService, - ) {} + ) { + this.discovery = new SyncFileDiscovery(dependencies, statuses); + this.resolver = new SyncStatusResolver(dependencies, statuses); + this.renameReconciler = new RenameReconciler( + { + settings: dependencies.settings, + syncManager: dependencies.syncManager, + refreshFileStatus: (path, remoteEntry) => this.resolver.refreshFileStatus(path, remoteEntry), + }, + statuses, + ); + } async refresh(onProgress?: (progress: SyncStatusRefreshProgress) => void): Promise { this.statuses.clear(); - const files = await this.discoverFiles(); - this.initializeFileStatuses(files.local); + const files = await this.discovery.discoverFiles(); + this.discovery.initializeFileStatuses(files.local); for (const hiddenPath of files.hiddenLocalPaths) { this.statuses.set(hiddenPath, { path: hiddenPath, status: 'checking' }); } - const extra = await this.identifyExtraFiles( + const extra = await this.discovery.identifyExtraFiles( files.remoteMap, files.localMap, files.allMap, - this.pendingMoveOldPaths(), + this.renameReconciler.pendingMoveOldPaths(), ); this.addExtraToStatuses(extra); - const filesToCheck = this.getCheckableFiles(files.local, extra, files.hiddenLocalPaths); - await this.performStatusCheck(filesToCheck, files.remoteMap, onProgress); - await this.reconcileOutOfBandMoves(files.remoteMap); + const filesToCheck = this.discovery.getCheckableFiles(files.local, extra, files.hiddenLocalPaths); + await this.resolver.performStatusCheck(filesToCheck, files.remoteMap, onProgress); + await this.renameReconciler.reconcileOutOfBandMoves(files.remoteMap); return { localCount: files.local.length + files.hiddenLocalPaths.size, @@ -85,192 +93,6 @@ export class SyncStatusRefreshService { }; } - async discoverFiles(): Promise { - const { app } = this.dependencies; - const settings = this.dependencies.settings(); - const gitService = this.dependencies.gitService(); - const gitignoreManager = this.dependencies.gitignoreManager(); - const allFiles = app.vault.getFiles(); - let local = this.dependencies.filterFilesByVaultFolder(allFiles); - const remoteHead = await gitService.getBranchHead?.(settings.branch); - const remoteEntries = await gitService.listFilesDetailed(remoteHead ?? settings.branch, false); - - await gitignoreManager.loadGitignores(remoteEntries); - - const remoteMap = new Map(); - const skipSymlinks = getEffectiveSymlinkHandling(settings) === 'skip'; - for (const entry of remoteEntries) { - if (entry.symlink && skipSymlinks) continue; - const normalized = this.getNormalizedRemotePath(entry.path); - if (normalized === null) continue; - - const vaultPath = this.dependencies.getVaultPath(normalized); - if (!gitignoreManager.isIgnored(normalized)) remoteMap.set(vaultPath, entry); - } - - local = local.filter(file => !gitignoreManager.isIgnored(this.dependencies.getNormalizedPath(file.path))); - const hiddenLocalPaths = await this.discoverHiddenLocalFiles(); - const filteredHiddenPaths = new Set( - hiddenLocalPaths - .filter(path => this.dependencies.filterPathByVaultFolder(path)) - .filter(path => !gitignoreManager.isIgnored(this.dependencies.getNormalizedPath(path))), - ); - - return { - local, - remoteEntries, - remoteHead, - remoteMap, - localMap: new Set([...local.map(file => file.path), ...filteredHiddenPaths]), - allMap: new Map(allFiles.map(file => [file.path, file])), - hiddenLocalPaths: filteredHiddenPaths, - }; - } - - getNormalizedRemotePath(remotePath: string): string | null { - const rootPath = this.dependencies.settings().rootPath; - if (!rootPath) return remotePath; - const cleanRoot = rootPath.endsWith('/') ? rootPath : `${rootPath}/`; - if (remotePath.startsWith(cleanRoot)) return remotePath.substring(cleanRoot.length); - return remotePath === rootPath ? '' : null; - } - - async discoverHiddenLocalFiles(): Promise { - const result: string[] = []; - await this.recursiveScan(this.dependencies.settings().vaultFolder || '', result); - return result; - } - - async recursiveScan(folderPath: string, result: string[]): Promise { - try { - const listing = await this.dependencies.app.vault.adapter.list(folderPath); - for (const file of listing.files) { - if (!this.isHidden(file)) continue; - if (readLocalSymlinkTarget(this.dependencies.app, file) !== null || await this.isLocalFile(file)) result.push(file); - } - for (const folder of listing.folders) { - if (folder === '.git' || folder.endsWith('/.git')) continue; - if (readLocalSymlinkTarget(this.dependencies.app, folder) !== null) { - if (this.isHidden(folder)) result.push(folder); - continue; - } - await this.recursiveScan(folder, result); - } - } catch { - // Some Obsidian adapters do not support raw directory listing. - } - } - - async identifyExtraFiles( - remoteMap: Map, - localFilePaths: Set, - allLocalFileMap: Map, - pendingMoveOldPaths: Set = new Set(), - ): Promise> { - const extra: Array = []; - for (const [vaultPath] of remoteMap) { - if (localFilePaths.has(vaultPath) || pendingMoveOldPaths.has(vaultPath)) continue; - - let localFile = allLocalFileMap.get(vaultPath); - if (!localFile) { - const abstractFile = this.dependencies.app.vault.getAbstractFileByPath(vaultPath); - if (abstractFile instanceof TFile) localFile = abstractFile; - } - - if (localFile) extra.push(localFile); - else if (await this.isLocalFile(vaultPath)) extra.push(vaultPath); - else { - // No local file at all. A tracked file that's since been - // removed locally (sync metadata still present for the path, - // and not a pending move source) is a *local deletion* — a - // potential remote deletion — distinct from a never-tracked - // remote-only file, which is simply available to download. - this.statuses.set(vaultPath, { - path: vaultPath, - status: this.statuses.classify({ - localExists: false, - remoteExists: true, - wasTracked: this.wasTrackedBeforeDelete(vaultPath), - }), - }); - } - } - return extra; - } - - /** - * Whether `vaultPath` was previously tracked locally and has since been - * removed (sync metadata present for the path, and not a pending move - * source). Used to distinguish a `local-deleted` row from a - * never-tracked `remote-only` download candidate. - */ - private wasTrackedBeforeDelete(vaultPath: string): boolean { - const metadata = this.dependencies.settings().syncMetadata; - const pathMetadata = metadata ? metadata[vaultPath] : undefined; - return isSyncMetadataAtPath(pathMetadata, vaultPath) && !pathMetadata.renamedFrom; - } - - /** The last-synced blob sha on record for `path`, or undefined if never tracked there. */ - private baseShaFor(path: string): string | undefined { - const metadata = this.dependencies.settings().syncMetadata; - const pathMetadata = metadata ? metadata[path] : undefined; - return isSyncMetadataAtPath(pathMetadata, path) ? pathMetadata.lastSyncedSha : undefined; - } - - /** - * Direction facts for a two-sided diff, relative to the last-synced - * baseline: undefined for both when there is no baseline on record (the - * two-sided diff then falls back to the direction-blind `modified`). - */ - private diffDirection(path: string, localSha: string, remoteSha: string): { localChanged?: boolean; remoteChanged?: boolean } { - const baseSha = this.baseShaFor(path); - if (baseSha === undefined) return {}; - return { localChanged: localSha !== baseSha, remoteChanged: remoteSha !== baseSha }; - } - - async reconcileOutOfBandMoves(remoteMap: Map): Promise { - const orphansBySha = this.orphanedMoveSourcesBySha(remoteMap); - if (orphansBySha.size === 0) return; - const candidatesBySha = await this.unsyncedMoveDestinationsBySha(remoteMap, orphansBySha); - - for (const [sha, orphanPaths] of orphansBySha) { - if (orphanPaths.length !== 1) continue; - const newPaths = candidatesBySha.get(sha); - if (!newPaths || newPaths.length !== 1) continue; - const oldPath = orphanPaths[0] as string; - const newPath = newPaths[0] as string; - await this.dependencies.syncManager().trackRename(newPath, oldPath); - this.statuses.delete(oldPath); - await this.refreshFileStatus(newPath, remoteMap.get(newPath)); - } - } - - async performStatusCheck( - filesToCheck: Array, - remoteMap: Map, - onProgress?: (progress: SyncStatusRefreshProgress) => void, - ): Promise { - const total = filesToCheck.length; - let current = 0; - let next = 0; - onProgress?.({ current, total }); - - const worker = async (): Promise => { - while (next < total) { - const file = filesToCheck[next++]; - if (file) { - const path = typeof file === 'string' ? file : file.path; - await this.refreshFileStatus(file, remoteMap.get(path), remoteMap); - } - current += 1; - onProgress?.({ current, total }); - } - }; - - const workerCount = Math.min(SyncStatusRefreshService.STATUS_CHECK_CONCURRENCY, total); - await Promise.all(Array.from({ length: workerCount }, () => worker())); - } - /** * Handles an out-of-band local create so a brand-new file appears in the * Source Control view immediately rather than waiting for the next full @@ -303,7 +125,7 @@ export class SyncStatusRefreshService { path: file.path, status: this.statuses.classify({ localExists: true, remoteExists: false }), }); - void this.readFileContent(file, isBinaryPath(file.path), false).then(localContent => { + void this.resolver.readFileContent(file, isBinaryPath(file.path), false).then(localContent => { const current = this.statuses.get(file.path); if (!current || current.file !== file @@ -315,18 +137,11 @@ export class SyncStatusRefreshService { return true; } - /** Monotonic per-path counter ordering async content reads so only the newest one may write. */ - private bumpContentRevision(path: string): number { - const next = (this.contentRevisions.get(path) ?? 0) + 1; - this.contentRevisions.set(path, next); - return next; - } - async handleFileModified(file: TFile): Promise { const existing = this.statuses.get(file.path); if (!existing || !['synced', 'modified', 'unsynced', 'moved'].includes(existing.status)) return false; const revision = this.bumpContentRevision(file.path); - const localContent = await this.readFileContent(file, isBinaryPath(file.path), false); + const localContent = await this.resolver.readFileContent(file, isBinaryPath(file.path), false); // A create's slow async read may still be in flight behind this // modify; only the newest read may write. if (this.contentRevisions.get(file.path) !== revision) return true; @@ -349,7 +164,7 @@ export class SyncStatusRefreshService { localExists: true, remoteExists: true, contentsEqual: localSha === remoteSha, - ...this.diffDirection(file.path, localSha, remoteSha), + ...this.resolver.diffDirection(file.path, localSha, remoteSha), }); } } @@ -418,151 +233,11 @@ export class SyncStatusRefreshService { return true; } - async refreshFileStatus( - fileOrPath: TFile | string, - remoteEntry: GitTreeEntry | undefined, - remoteMap?: Map, - ): Promise { - try { - const path = typeof fileOrPath === 'string' ? fileOrPath : fileOrPath.path; - const renamedFrom = this.dependencies.settings().syncMetadata?.[path]?.renamedFrom; - if (renamedFrom !== undefined) { - await this.refreshMovedFileStatus(fileOrPath, renamedFrom, remoteMap?.get(renamedFrom)); - } else if (remoteEntry === undefined) { - await this.refreshLocalOnlyStatus(fileOrPath); - } else if (remoteEntry.sha !== undefined) { - await this.refreshFileStatusBySha(fileOrPath, remoteEntry); - } else { - await this.refreshFileStatusByContent(fileOrPath); - } - } catch (error) { - const path = typeof fileOrPath === 'string' ? fileOrPath : fileOrPath.path; - logger.warn(`Failed to determine sync status for ${path}`, error); - this.statuses.set(path, { - file: typeof fileOrPath === 'string' ? undefined : fileOrPath, - path, - status: this.statuses.classify({ localExists: true, remoteExists: false }), - }); - } - } - - async refreshFileStatusBySha(fileOrPath: TFile | string, remoteEntry: GitTreeEntry): Promise { - const isStringPath = typeof fileOrPath === 'string'; - const path = isStringPath ? fileOrPath : fileOrPath.path; - const file = isStringPath ? undefined : fileOrPath; - const binary = isBinaryPath(path); - const symlinkMode = getEffectiveSymlinkHandling(this.dependencies.settings()); - const localContent = await this.readLocalContentForSha(fileOrPath, isStringPath, binary, remoteEntry.symlink, symlinkMode); - const localSha = await gitBlobSha(localContent); - const remoteSha = remoteEntry.sha; - const status = this.statuses.classify({ - localExists: true, - remoteExists: true, - contentsEqual: localSha === remoteSha, - ...(remoteSha !== undefined ? this.diffDirection(path, localSha, remoteSha) : {}), - }); - if (status === 'synced' && remoteEntry.sha) { - await this.dependencies.syncManager().updateMetadata(path, remoteEntry.sha); - } - this.statuses.set(path, { - file, - path, - status, - localContent, - remoteSha: remoteEntry.sha, - isSymlink: remoteEntry.symlink, - }); - } - - async refreshFileStatusByContent(fileOrPath: TFile | string): Promise { - const isStringPath = typeof fileOrPath === 'string'; - const path = isStringPath ? fileOrPath : fileOrPath.path; - const file = isStringPath ? undefined : fileOrPath; - const localContent = await this.readFileContent(fileOrPath, isBinaryPath(path), isStringPath); - const remote = await this.dependencies.gitService().getFile( - this.dependencies.getNormalizedPath(path), - this.dependencies.settings().branch, - ); - let status: FileStatus['status']; - if (!remote.sha) { - status = this.statuses.classify({ localExists: true, remoteExists: false }); - } else { - const equal = contentsEqual(localContent, remote.content); - status = this.statuses.classify({ - localExists: true, - remoteExists: true, - contentsEqual: equal, - ...(equal ? {} : this.diffDirection(path, await gitBlobSha(localContent), remote.sha)), - }); - } - if (status === 'synced' && remote.sha) { - await this.dependencies.syncManager().updateMetadata(path, remote.sha); - } - this.statuses.set(path, { - file, - path, - status, - localContent, - remoteContent: remote.content, - remoteSha: remote.sha, - }); - } - - private isHidden(path: string): boolean { - return path.split('/').some(part => part.startsWith('.')); - } - - private async isLocalFile(vaultPath: string): Promise { - const stat = await this.dependencies.app.vault.adapter.stat(vaultPath); - return stat?.type === 'file'; - } - - private initializeFileStatuses(localFiles: TFile[]): void { - for (const file of localFiles) this.statuses.set(file.path, { file, path: file.path, status: 'checking' }); - } - - private pendingMoveOldPaths(): Set { - const paths = new Set(); - for (const metadata of Object.values(this.dependencies.settings().syncMetadata ?? {})) { - if (metadata.renamedFrom) paths.add(metadata.renamedFrom); - } - return paths; - } - - private orphanedMoveSourcesBySha(remoteMap: Map): Map { - const metadata = this.dependencies.settings().syncMetadata ?? {}; - const orphansBySha = new Map(); - for (const [path, status] of this.statuses) { - // A tracked-then-deleted file is now classified `local-deleted` - // (not `remote-only`), so both qualify as an orphaned move - // source: the remote entry still exists, sync metadata is - // present for the path, and it isn't itself a pending move. - if (status.status !== 'remote-only' && status.status !== 'local-deleted') continue; - const pathMetadata = metadata[path]; - if (!isSyncMetadataAtPath(pathMetadata, path) || pathMetadata.renamedFrom) continue; - const entry = remoteMap.get(path); - if (!entry || entry.symlink || !entry.sha) continue; - const paths = orphansBySha.get(entry.sha) ?? []; - paths.push(path); - orphansBySha.set(entry.sha, paths); - } - return orphansBySha; - } - - private async unsyncedMoveDestinationsBySha( - remoteMap: Map, - orphansBySha: Map, - ): Promise> { - const candidatesBySha = new Map(); - for (const [path, status] of this.statuses) { - if (status.status !== 'unsynced' || status.localContent === undefined || remoteMap.has(path)) continue; - const sha = await gitBlobSha(status.localContent); - if (!orphansBySha.has(sha)) continue; - const paths = candidatesBySha.get(sha) ?? []; - paths.push(path); - candidatesBySha.set(sha, paths); - } - return candidatesBySha; + /** Monotonic per-path counter ordering async content reads so only the newest one may write. */ + private bumpContentRevision(path: string): number { + const next = (this.contentRevisions.get(path) ?? 0) + 1; + this.contentRevisions.set(path, next); + return next; } private addExtraToStatuses(extra: Array): void { @@ -575,87 +250,4 @@ export class SyncStatusRefreshService { }); } } - - private getCheckableFiles( - local: TFile[], - extra: Array, - hiddenLocalPaths: Set, - ): Array { - const extraPaths = new Set(extra.map(file => typeof file === 'string' ? file : file.path)); - const hiddenToAdd = [...hiddenLocalPaths].filter(path => !extraPaths.has(path)); - const gitignoreManager = this.dependencies.gitignoreManager(); - return [...local, ...extra, ...hiddenToAdd].filter(file => { - const path = typeof file === 'string' ? file : file.path; - return !gitignoreManager.isIgnored(this.dependencies.getNormalizedPath(path)); - }); - } - - private async refreshMovedFileStatus(fileOrPath: TFile | string, movedFrom: string, sourceEntry?: GitTreeEntry): Promise { - const isStringPath = typeof fileOrPath === 'string'; - const path = isStringPath ? fileOrPath : fileOrPath.path; - const localContent = await this.readFileContent(fileOrPath, isBinaryPath(path), isStringPath); - this.statuses.set(path, { - file: isStringPath ? undefined : fileOrPath, - path, - status: this.statuses.classify({ movedFrom }), - movedFrom, - localContent, - remoteSha: sourceEntry?.sha, - isSymlink: sourceEntry?.symlink, - }); - } - - private async refreshLocalOnlyStatus(fileOrPath: TFile | string): Promise { - const isStringPath = typeof fileOrPath === 'string'; - const path = isStringPath ? fileOrPath : fileOrPath.path; - const localContent = await this.readFileContent(fileOrPath, isBinaryPath(path), isStringPath); - this.statuses.set(path, { - file: isStringPath ? undefined : fileOrPath, - path, - status: this.statuses.classify({ localExists: true, remoteExists: false }), - localContent, - }); - } - - private async readLocalContentForSha( - fileOrPath: TFile | string, - isStringPath: boolean, - binary: boolean, - remoteIsSymlink: boolean, - symlinkMode: SymlinkHandling, - ): Promise { - if (remoteIsSymlink && symlinkMode === 'real') { - const path = typeof fileOrPath === 'string' ? fileOrPath : fileOrPath.path; - const target = readLocalSymlinkTarget(this.dependencies.app, path); - if (target !== null) return target; - } - return this.readFileContent(fileOrPath, binary, isStringPath); - } - - private async readFileContent(fileOrPath: TFile | string, binary: boolean, isStringPath: boolean): Promise { - if (isStringPath) return this.readStringPathContent(fileOrPath as string, binary); - if (!(fileOrPath instanceof TFile)) throw new Error('Expected TFile when isStringPath is false'); - try { - return binary - ? await this.dependencies.app.vault.readBinary(fileOrPath) - : await this.dependencies.app.vault.read(fileOrPath); - } catch (error) { - logger.warn(`vault.read failed for ${fileOrPath.path}; falling back to adapter`, error); - return binary - ? await this.dependencies.app.vault.adapter.readBinary(fileOrPath.path) - : await this.dependencies.app.vault.adapter.read(fileOrPath.path); - } - } - - private async readStringPathContent(path: string, binary: boolean): Promise { - try { - return binary - ? await this.dependencies.app.vault.adapter.readBinary(path) - : await this.dependencies.app.vault.adapter.read(path); - } catch (error) { - const target = readLocalSymlinkTarget(this.dependencies.app, path); - if (target !== null) return target; - throw error; - } - } } diff --git a/src/logic/sync/SyncStatusResolver.ts b/src/logic/sync/SyncStatusResolver.ts new file mode 100644 index 0000000..36ce178 --- /dev/null +++ b/src/logic/sync/SyncStatusResolver.ts @@ -0,0 +1,239 @@ +import { type App, TFile } from 'obsidian'; +import { getEffectiveSymlinkHandling, isSyncMetadataAtPath, type GitLabFilesPushSettings, type SymlinkHandling } from '../../settings'; +import { type FileStatus, type SyncStatusService } from '../sync-status-service'; +import type { GitServiceInterface, GitTreeEntry } from '../../services/git-service-interface'; +import { gitBlobSha } from '../../utils/git-blob-sha'; +import { logger } from '../../utils/logger'; +import { contentsEqual, isBinaryPath } from '../../utils/path'; +import { readLocalSymlinkTarget } from '../../utils/symlink'; +import type { SyncManager } from './SyncManager'; + +export interface SyncStatusResolverDependencies { + app: App; + settings: () => GitLabFilesPushSettings; + gitService: () => GitServiceInterface; + syncManager: () => SyncManager; + getNormalizedPath(path: string): string; +} + +export interface SyncStatusResolverProgress { + current: number; + total: number; +} + +/** + * Resolves a file's sync status: local-vs-remote diff, SHA/content + * comparison, baseline direction, and `FileStatus` classification. + * Deliberately owns no discovery or rename-reconciliation logic. + */ +export class SyncStatusResolver { + private static readonly STATUS_CHECK_CONCURRENCY = 8; + + constructor( + private readonly dependencies: SyncStatusResolverDependencies, + private readonly statuses: SyncStatusService, + ) {} + + async performStatusCheck( + filesToCheck: Array, + remoteMap: Map, + onProgress?: (progress: SyncStatusResolverProgress) => void, + ): Promise { + const total = filesToCheck.length; + let current = 0; + let next = 0; + onProgress?.({ current, total }); + + const worker = async (): Promise => { + while (next < total) { + const file = filesToCheck[next++]; + if (file) { + const path = typeof file === 'string' ? file : file.path; + await this.refreshFileStatus(file, remoteMap.get(path), remoteMap); + } + current += 1; + onProgress?.({ current, total }); + } + }; + + const workerCount = Math.min(SyncStatusResolver.STATUS_CHECK_CONCURRENCY, total); + await Promise.all(Array.from({ length: workerCount }, () => worker())); + } + + async refreshFileStatus( + fileOrPath: TFile | string, + remoteEntry: GitTreeEntry | undefined, + remoteMap?: Map, + ): Promise { + try { + const path = typeof fileOrPath === 'string' ? fileOrPath : fileOrPath.path; + const renamedFrom = this.dependencies.settings().syncMetadata?.[path]?.renamedFrom; + if (renamedFrom !== undefined) { + await this.refreshMovedFileStatus(fileOrPath, renamedFrom, remoteMap?.get(renamedFrom)); + } else if (remoteEntry === undefined) { + await this.refreshLocalOnlyStatus(fileOrPath); + } else if (remoteEntry.sha !== undefined) { + await this.refreshFileStatusBySha(fileOrPath, remoteEntry); + } else { + await this.refreshFileStatusByContent(fileOrPath); + } + } catch (error) { + const path = typeof fileOrPath === 'string' ? fileOrPath : fileOrPath.path; + logger.warn(`Failed to determine sync status for ${path}`, error); + this.statuses.set(path, { + file: typeof fileOrPath === 'string' ? undefined : fileOrPath, + path, + status: this.statuses.classify({ localExists: true, remoteExists: false }), + }); + } + } + + async refreshFileStatusBySha(fileOrPath: TFile | string, remoteEntry: GitTreeEntry): Promise { + const isStringPath = typeof fileOrPath === 'string'; + const path = isStringPath ? fileOrPath : fileOrPath.path; + const file = isStringPath ? undefined : fileOrPath; + const binary = isBinaryPath(path); + const symlinkMode = getEffectiveSymlinkHandling(this.dependencies.settings()); + const localContent = await this.readLocalContentForSha(fileOrPath, isStringPath, binary, remoteEntry.symlink, symlinkMode); + const localSha = await gitBlobSha(localContent); + const remoteSha = remoteEntry.sha; + const status = this.statuses.classify({ + localExists: true, + remoteExists: true, + contentsEqual: localSha === remoteSha, + ...(remoteSha !== undefined ? this.diffDirection(path, localSha, remoteSha) : {}), + }); + if (status === 'synced' && remoteEntry.sha) { + await this.dependencies.syncManager().updateMetadata(path, remoteEntry.sha); + } + this.statuses.set(path, { + file, + path, + status, + localContent, + remoteSha: remoteEntry.sha, + isSymlink: remoteEntry.symlink, + }); + } + + async refreshFileStatusByContent(fileOrPath: TFile | string): Promise { + const isStringPath = typeof fileOrPath === 'string'; + const path = isStringPath ? fileOrPath : fileOrPath.path; + const file = isStringPath ? undefined : fileOrPath; + const localContent = await this.readFileContent(fileOrPath, isBinaryPath(path), isStringPath); + const remote = await this.dependencies.gitService().getFile( + this.dependencies.getNormalizedPath(path), + this.dependencies.settings().branch, + ); + let status: FileStatus['status']; + if (!remote.sha) { + status = this.statuses.classify({ localExists: true, remoteExists: false }); + } else { + const equal = contentsEqual(localContent, remote.content); + status = this.statuses.classify({ + localExists: true, + remoteExists: true, + contentsEqual: equal, + ...(equal ? {} : this.diffDirection(path, await gitBlobSha(localContent), remote.sha)), + }); + } + if (status === 'synced' && remote.sha) { + await this.dependencies.syncManager().updateMetadata(path, remote.sha); + } + this.statuses.set(path, { + file, + path, + status, + localContent, + remoteContent: remote.content, + remoteSha: remote.sha, + }); + } + + /** + * Direction facts for a two-sided diff, relative to the last-synced + * baseline: undefined for both when there is no baseline on record (the + * two-sided diff then falls back to the direction-blind `modified`). + */ + diffDirection(path: string, localSha: string, remoteSha: string): { localChanged?: boolean; remoteChanged?: boolean } { + const baseSha = this.baseShaFor(path); + if (baseSha === undefined) return {}; + return { localChanged: localSha !== baseSha, remoteChanged: remoteSha !== baseSha }; + } + + async readFileContent(fileOrPath: TFile | string, binary: boolean, isStringPath: boolean): Promise { + if (isStringPath) return this.readStringPathContent(fileOrPath as string, binary); + if (!(fileOrPath instanceof TFile)) throw new Error('Expected TFile when isStringPath is false'); + try { + return binary + ? await this.dependencies.app.vault.readBinary(fileOrPath) + : await this.dependencies.app.vault.read(fileOrPath); + } catch (error) { + logger.warn(`vault.read failed for ${fileOrPath.path}; falling back to adapter`, error); + return binary + ? await this.dependencies.app.vault.adapter.readBinary(fileOrPath.path) + : await this.dependencies.app.vault.adapter.read(fileOrPath.path); + } + } + + /** The last-synced blob sha on record for `path`, or undefined if never tracked there. */ + private baseShaFor(path: string): string | undefined { + const metadata = this.dependencies.settings().syncMetadata; + const pathMetadata = metadata ? metadata[path] : undefined; + return isSyncMetadataAtPath(pathMetadata, path) ? pathMetadata.lastSyncedSha : undefined; + } + + private async refreshMovedFileStatus(fileOrPath: TFile | string, movedFrom: string, sourceEntry?: GitTreeEntry): Promise { + const isStringPath = typeof fileOrPath === 'string'; + const path = isStringPath ? fileOrPath : fileOrPath.path; + const localContent = await this.readFileContent(fileOrPath, isBinaryPath(path), isStringPath); + this.statuses.set(path, { + file: isStringPath ? undefined : fileOrPath, + path, + status: this.statuses.classify({ movedFrom }), + movedFrom, + localContent, + remoteSha: sourceEntry?.sha, + isSymlink: sourceEntry?.symlink, + }); + } + + private async refreshLocalOnlyStatus(fileOrPath: TFile | string): Promise { + const isStringPath = typeof fileOrPath === 'string'; + const path = isStringPath ? fileOrPath : fileOrPath.path; + const localContent = await this.readFileContent(fileOrPath, isBinaryPath(path), isStringPath); + this.statuses.set(path, { + file: isStringPath ? undefined : fileOrPath, + path, + status: this.statuses.classify({ localExists: true, remoteExists: false }), + localContent, + }); + } + + private async readLocalContentForSha( + fileOrPath: TFile | string, + isStringPath: boolean, + binary: boolean, + remoteIsSymlink: boolean, + symlinkMode: SymlinkHandling, + ): Promise { + if (remoteIsSymlink && symlinkMode === 'real') { + const path = typeof fileOrPath === 'string' ? fileOrPath : fileOrPath.path; + const target = readLocalSymlinkTarget(this.dependencies.app, path); + if (target !== null) return target; + } + return this.readFileContent(fileOrPath, binary, isStringPath); + } + + private async readStringPathContent(path: string, binary: boolean): Promise { + try { + return binary + ? await this.dependencies.app.vault.adapter.readBinary(path) + : await this.dependencies.app.vault.adapter.read(path); + } catch (error) { + const target = readLocalSymlinkTarget(this.dependencies.app, path); + if (target !== null) return target; + throw error; + } + } +} diff --git a/tests/logic/sync/RenameReconciler.test.ts b/tests/logic/sync/RenameReconciler.test.ts new file mode 100644 index 0000000..c8d0252 --- /dev/null +++ b/tests/logic/sync/RenameReconciler.test.ts @@ -0,0 +1,105 @@ +import { describe, expect, it, vi } from 'vitest'; +import { RenameReconciler } from '../../../src/logic/sync/RenameReconciler'; +import type { RenameReconcilerDependencies } from '../../../src/logic/sync/RenameReconciler'; +import { SyncStatusService } from '../../../src/logic/sync-status-service'; +import { gitBlobSha } from '../../../src/utils/git-blob-sha'; + +function buildReconciler(statuses: SyncStatusService, deps: Partial = {}): { + reconciler: RenameReconciler; + trackRename: ReturnType; + refreshFileStatus: ReturnType; +} { + const trackRename = vi.fn().mockResolvedValue(undefined); + const refreshFileStatus = vi.fn().mockResolvedValue(undefined); + const base: RenameReconcilerDependencies = { + settings: () => ({ syncMetadata: {} }) as never, + syncManager: () => ({ trackRename }) as never, + refreshFileStatus, + }; + return { reconciler: new RenameReconciler({ ...base, ...deps }, statuses), trackRename, refreshFileStatus }; +} + +describe('RenameReconciler', () => { + describe('reconcileOutOfBandMoves', () => { + it('tracks a rename when exactly one orphaned tracked path matches exactly one unsynced local file by blob sha', async () => { + const statuses = new SyncStatusService(); + const content = 'moved content'; + const sha = await gitBlobSha(content); + statuses.set({ path: 'old.md', status: 'local-deleted' }); + statuses.set({ path: 'new.md', status: 'unsynced', localContent: content }); + const remoteMap = new Map([ + ['old.md', { path: 'old.md', sha, symlink: false }], + ]); + const { reconciler, trackRename, refreshFileStatus } = buildReconciler(statuses, { + settings: () => ({ syncMetadata: { 'old.md': { lastSyncedSha: sha, lastSyncedAt: 1 } } }) as never, + }); + + await reconciler.reconcileOutOfBandMoves(remoteMap); + + expect(trackRename).toHaveBeenCalledWith('new.md', 'old.md'); + expect(statuses.has('old.md')).toBe(false); + expect(refreshFileStatus).toHaveBeenCalledWith('new.md', undefined); + }); + + it('does nothing when a sha has more than one orphaned candidate (ambiguous match)', async () => { + const statuses = new SyncStatusService(); + const content = 'moved content'; + const sha = await gitBlobSha(content); + statuses.set({ path: 'old-a.md', status: 'local-deleted' }); + statuses.set({ path: 'old-b.md', status: 'local-deleted' }); + statuses.set({ path: 'new.md', status: 'unsynced', localContent: content }); + const remoteMap = new Map([ + ['old-a.md', { path: 'old-a.md', sha, symlink: false }], + ['old-b.md', { path: 'old-b.md', sha, symlink: false }], + ]); + const { reconciler, trackRename } = buildReconciler(statuses, { + settings: () => ({ + syncMetadata: { + 'old-a.md': { lastSyncedSha: sha, lastSyncedAt: 1 }, + 'old-b.md': { lastSyncedSha: sha, lastSyncedAt: 1 }, + }, + }) as never, + }); + + await reconciler.reconcileOutOfBandMoves(remoteMap); + + expect(trackRename).not.toHaveBeenCalled(); + }); + + it('ignores an orphan candidate that is itself a pending move source (renamedFrom set)', async () => { + const statuses = new SyncStatusService(); + const content = 'content'; + const sha = await gitBlobSha(content); + statuses.set({ path: 'old.md', status: 'local-deleted' }); + statuses.set({ path: 'new.md', status: 'unsynced', localContent: content }); + const remoteMap = new Map([ + ['old.md', { path: 'old.md', sha, symlink: false }], + ]); + const { reconciler, trackRename } = buildReconciler(statuses, { + settings: () => ({ + syncMetadata: { 'old.md': { lastSyncedSha: sha, lastSyncedAt: 1, renamedFrom: 'older.md' } }, + }) as never, + }); + + await reconciler.reconcileOutOfBandMoves(remoteMap); + + expect(trackRename).not.toHaveBeenCalled(); + }); + }); + + describe('pendingMoveOldPaths', () => { + it('collects every renamedFrom source path currently on record', () => { + const statuses = new SyncStatusService(); + const { reconciler } = buildReconciler(statuses, { + settings: () => ({ + syncMetadata: { + 'a.md': { lastSyncedSha: 'x', lastSyncedAt: 1, renamedFrom: 'a-old.md' }, + 'b.md': { lastSyncedSha: 'y', lastSyncedAt: 1 }, + }, + }) as never, + }); + + expect(reconciler.pendingMoveOldPaths()).toEqual(new Set(['a-old.md'])); + }); + }); +}); diff --git a/tests/logic/sync/SyncFileDiscovery.test.ts b/tests/logic/sync/SyncFileDiscovery.test.ts new file mode 100644 index 0000000..59b7499 --- /dev/null +++ b/tests/logic/sync/SyncFileDiscovery.test.ts @@ -0,0 +1,124 @@ +import { describe, expect, it, vi } from 'vitest'; +import { TFile } from 'obsidian'; +import { SyncFileDiscovery } from '../../../src/logic/sync/SyncFileDiscovery'; +import type { SyncFileDiscoveryDependencies } from '../../../src/logic/sync/SyncFileDiscovery'; +import { SyncStatusService } from '../../../src/logic/sync-status-service'; + +vi.mock('obsidian'); + +function buildDiscovery(statuses: SyncStatusService, deps: Partial = {}): SyncFileDiscovery { + const base: SyncFileDiscoveryDependencies = { + app: { + vault: { + adapter: { stat: vi.fn().mockResolvedValue(null) }, + getAbstractFileByPath: vi.fn().mockReturnValue(null), + }, + } as never, + settings: () => ({ syncMetadata: {}, vaultFolder: '', rootPath: '' }) as never, + gitService: () => ({}) as never, + gitignoreManager: () => ({ isIgnored: () => false }) as never, + filterFilesByVaultFolder: files => files, + filterPathByVaultFolder: () => true, + getNormalizedPath: path => path, + getVaultPath: path => path, + }; + return new SyncFileDiscovery({ ...base, ...deps }, statuses); +} + +describe('SyncFileDiscovery', () => { + describe('identifyExtraFiles local-deleted classification', () => { + it('classifies a previously-tracked removed file as local-deleted', async () => { + const statuses = new SyncStatusService(); + const remoteMap = new Map([['note.md', { path: 'note.md', sha: 'abc', symlink: false }]]); + const discovery = buildDiscovery(statuses, { + settings: () => ({ + syncMetadata: { 'note.md': { sha: 'abc', lastSyncedAt: 1, renamedFrom: undefined } }, + vaultFolder: '', + rootPath: '', + }) as never, + }); + + await discovery.identifyExtraFiles(remoteMap, new Set(), new Map()); + + expect(statuses.get('note.md')?.status).toBe('local-deleted'); + }); + + it('classifies a never-tracked remote-only file as remote-only', async () => { + const statuses = new SyncStatusService(); + const remoteMap = new Map([['remote.md', { path: 'remote.md', sha: 'abc', symlink: false }]]); + const discovery = buildDiscovery(statuses, { + settings: () => ({ syncMetadata: {}, vaultFolder: '', rootPath: '' }) as never, + }); + + await discovery.identifyExtraFiles(remoteMap, new Set(), new Map()); + + expect(statuses.get('remote.md')?.status).toBe('remote-only'); + }); + + it('treats a path with a pending rename (renamedFrom) as remote-only, not local-deleted', async () => { + const statuses = new SyncStatusService(); + const remoteMap = new Map([['note.md', { path: 'note.md', sha: 'abc', symlink: false }]]); + const discovery = buildDiscovery(statuses, { + settings: () => ({ + syncMetadata: { 'note.md': { sha: 'abc', lastSyncedAt: 1, renamedFrom: 'old.md' } }, + vaultFolder: '', + rootPath: '', + }) as never, + }); + + await discovery.identifyExtraFiles(remoteMap, new Set(), new Map()); + + expect(statuses.get('note.md')?.status).toBe('remote-only'); + }); + + it('leaves an in-scope local file alone (returned as an extra candidate, not classified)', async () => { + const statuses = new SyncStatusService(); + const file = new TFile(); + file.path = 'note.md'; + const remoteMap = new Map([['note.md', { path: 'note.md', sha: 'abc', symlink: false }]]); + const discovery = buildDiscovery(statuses); + + const extra = await discovery.identifyExtraFiles(remoteMap, new Set(), new Map([['note.md', file]])); + + expect(extra).toEqual([file]); + expect(statuses.has('note.md')).toBe(false); + }); + }); + + describe('discoverFiles', () => { + it('excludes gitignored local and remote paths and normalizes remote paths under rootPath', async () => { + const statuses = new SyncStatusService(); + const localFile = new TFile(); + localFile.path = 'keep.md'; + const ignoredFile = new TFile(); + ignoredFile.path = 'ignored.md'; + const discovery = buildDiscovery(statuses, { + app: { + vault: { + getFiles: () => [localFile, ignoredFile], + adapter: { list: vi.fn().mockRejectedValue(new Error('no raw listing')) }, + }, + } as never, + settings: () => ({ syncMetadata: {}, vaultFolder: '', rootPath: 'vault' }) as never, + gitService: () => ({ + listFilesDetailed: vi.fn().mockResolvedValue([ + { path: 'vault/keep.md', sha: 'a', symlink: false }, + { path: 'other/outside.md', sha: 'b', symlink: false }, + ]), + }) as never, + gitignoreManager: () => ({ + loadGitignores: vi.fn().mockResolvedValue(undefined), + isIgnored: (path: string) => path === 'ignored.md', + }) as never, + filterFilesByVaultFolder: files => files, + }); + + const result = await discovery.discoverFiles(); + + expect(result.local.map(f => f.path)).toEqual(['keep.md']); + expect(result.remoteMap.has('keep.md')).toBe(true); + // Remote path outside rootPath is dropped entirely (getNormalizedRemotePath -> null). + expect(result.remoteMap.size).toBe(1); + }); + }); +}); diff --git a/tests/logic/sync/SyncStatusRefreshService.test.ts b/tests/logic/sync/SyncStatusRefreshService.test.ts index a4dcd95..7fdd934 100644 --- a/tests/logic/sync/SyncStatusRefreshService.test.ts +++ b/tests/logic/sync/SyncStatusRefreshService.test.ts @@ -378,50 +378,4 @@ describe('SyncStatusRefreshService local-change handlers', () => { expect(statuses.get('note.md')?.status).toBe('remote-modified'); }); }); - - describe('identifyExtraFiles local-deleted classification', () => { - it('classifies a previously-tracked removed file as local-deleted', async () => { - const statuses = new SyncStatusService(); - const remoteMap = new Map([['note.md', { path: 'note.md', sha: 'abc', symlink: false }]]); - const service = buildService(statuses, { - settings: () => ({ - syncMetadata: { 'note.md': { sha: 'abc', lastSyncedAt: 1, renamedFrom: undefined } }, - vaultFolder: '', - rootPath: '', - }) as never, - }); - - await service.identifyExtraFiles(remoteMap, new Set(), new Map()); - - expect(statuses.get('note.md')?.status).toBe('local-deleted'); - }); - - it('classifies a never-tracked remote-only file as remote-only', async () => { - const statuses = new SyncStatusService(); - const remoteMap = new Map([['remote.md', { path: 'remote.md', sha: 'abc', symlink: false }]]); - const service = buildService(statuses, { - settings: () => ({ syncMetadata: {}, vaultFolder: '', rootPath: '' }) as never, - }); - - await service.identifyExtraFiles(remoteMap, new Set(), new Map()); - - expect(statuses.get('remote.md')?.status).toBe('remote-only'); - }); - - it('treats a path with a pending rename (renamedFrom) as remote-only, not local-deleted', async () => { - const statuses = new SyncStatusService(); - const remoteMap = new Map([['note.md', { path: 'note.md', sha: 'abc', symlink: false }]]); - const service = buildService(statuses, { - settings: () => ({ - syncMetadata: { 'note.md': { sha: 'abc', lastSyncedAt: 1, renamedFrom: 'old.md' } }, - vaultFolder: '', - rootPath: '', - }) as never, - }); - - await service.identifyExtraFiles(remoteMap, new Set(), new Map()); - - expect(statuses.get('note.md')?.status).toBe('remote-only'); - }); - }); }); \ No newline at end of file diff --git a/tests/logic/sync/SyncStatusResolver.test.ts b/tests/logic/sync/SyncStatusResolver.test.ts new file mode 100644 index 0000000..8a8efdb --- /dev/null +++ b/tests/logic/sync/SyncStatusResolver.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, it, vi } from 'vitest'; +import { TFile } from 'obsidian'; +import { SyncStatusResolver } from '../../../src/logic/sync/SyncStatusResolver'; +import type { SyncStatusResolverDependencies } from '../../../src/logic/sync/SyncStatusResolver'; +import { SyncStatusService } from '../../../src/logic/sync-status-service'; +import { gitBlobSha } from '../../../src/utils/git-blob-sha'; + +vi.mock('obsidian'); + +function buildResolver(statuses: SyncStatusService, deps: Partial = {}): SyncStatusResolver { + const base: SyncStatusResolverDependencies = { + app: { + vault: { + read: vi.fn().mockResolvedValue(''), + readBinary: vi.fn(), + adapter: { read: vi.fn(), readBinary: vi.fn() }, + }, + } as never, + settings: () => ({ syncMetadata: {}, vaultFolder: '', rootPath: '', branch: 'main' }) as never, + gitService: () => ({}) as never, + syncManager: () => ({ updateMetadata: vi.fn().mockResolvedValue(undefined) }) as never, + getNormalizedPath: path => path, + }; + return new SyncStatusResolver({ ...base, ...deps }, statuses); +} + +function makeFile(path: string): TFile { + const file = new TFile(); + file.path = path; + return file; +} + +describe('SyncStatusResolver', () => { + describe('refreshFileStatusBySha', () => { + it('classifies synced when local content hashes to the remote sha, and updates metadata', async () => { + const statuses = new SyncStatusService(); + const content = 'hello world'; + const sha = await gitBlobSha(content); + const updateMetadata = vi.fn().mockResolvedValue(undefined); + const resolver = buildResolver(statuses, { + app: { vault: { read: vi.fn().mockResolvedValue(content), readBinary: vi.fn(), adapter: {} } } as never, + syncManager: () => ({ updateMetadata }) as never, + }); + const file = makeFile('note.md'); + + await resolver.refreshFileStatusBySha(file, { path: 'note.md', sha, symlink: false }); + + expect(statuses.get('note.md')?.status).toBe('synced'); + expect(updateMetadata).toHaveBeenCalledWith('note.md', sha); + }); + + it('classifies modified when local content differs from remote and there is no baseline sha on record', async () => { + const statuses = new SyncStatusService(); + const resolver = buildResolver(statuses, { + app: { vault: { read: vi.fn().mockResolvedValue('local content'), readBinary: vi.fn(), adapter: {} } } as never, + }); + const file = makeFile('note.md'); + + await resolver.refreshFileStatusBySha(file, { path: 'note.md', sha: 'b'.repeat(40), symlink: false }); + + expect(statuses.get('note.md')?.status).toBe('modified'); + }); + + it('classifies remote-modified when local content still matches the last-synced baseline but the remote sha moved', async () => { + const statuses = new SyncStatusService(); + const baselineContent = 'baseline content'; + const baselineSha = await gitBlobSha(baselineContent); + const resolver = buildResolver(statuses, { + app: { vault: { read: vi.fn().mockResolvedValue(baselineContent), readBinary: vi.fn(), adapter: {} } } as never, + settings: () => ({ + syncMetadata: { 'note.md': { lastSyncedSha: baselineSha, lastSyncedAt: 1 } }, + vaultFolder: '', + rootPath: '', + branch: 'main', + }) as never, + }); + const file = makeFile('note.md'); + + await resolver.refreshFileStatusBySha(file, { path: 'note.md', sha: 'c'.repeat(40), symlink: false }); + + expect(statuses.get('note.md')?.status).toBe('remote-modified'); + }); + }); + + describe('refreshFileStatusByContent', () => { + it('falls back to gitService.getFile content comparison when the remote entry has no sha', async () => { + const statuses = new SyncStatusService(); + const resolver = buildResolver(statuses, { + app: { vault: { read: vi.fn().mockResolvedValue('same content'), readBinary: vi.fn(), adapter: {} } } as never, + gitService: () => ({ + getFile: vi.fn().mockResolvedValue({ content: 'same content', sha: 'z'.repeat(40) }), + }) as never, + }); + const file = makeFile('note.md'); + + await resolver.refreshFileStatusByContent(file); + + expect(statuses.get('note.md')?.status).toBe('synced'); + }); + + it('classifies unsynced when the remote file does not exist (no sha)', async () => { + const statuses = new SyncStatusService(); + const resolver = buildResolver(statuses, { + app: { vault: { read: vi.fn().mockResolvedValue('content'), readBinary: vi.fn(), adapter: {} } } as never, + gitService: () => ({ getFile: vi.fn().mockResolvedValue({ content: undefined, sha: undefined }) }) as never, + }); + const file = makeFile('note.md'); + + await resolver.refreshFileStatusByContent(file); + + expect(statuses.get('note.md')?.status).toBe('unsynced'); + }); + }); + + describe('diffDirection', () => { + it('returns no direction facts when there is no baseline sha on record', () => { + const statuses = new SyncStatusService(); + const resolver = buildResolver(statuses, { settings: () => ({ syncMetadata: {}, vaultFolder: '', rootPath: '' }) as never }); + + expect(resolver.diffDirection('note.md', 'local-sha', 'remote-sha')).toEqual({}); + }); + + it('reports local/remote changed facts relative to the last-synced baseline', () => { + const statuses = new SyncStatusService(); + const resolver = buildResolver(statuses, { + settings: () => ({ + syncMetadata: { 'note.md': { lastSyncedSha: 'base-sha', lastSyncedAt: 1 } }, + vaultFolder: '', + rootPath: '', + }) as never, + }); + + expect(resolver.diffDirection('note.md', 'local-sha', 'base-sha')).toEqual({ localChanged: true, remoteChanged: false }); + }); + }); +}); From 9f2c1454fa49b4551d4e494c7d3cfbb9a78d8fd5 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 04:49:06 +0000 Subject: [PATCH 2/5] refactor(runtime): extract createSyncRuntime as the sync/Source Control composition root Move the sync-domain and Source Control application constructor graph (SyncManager, SyncStatusRefreshService, SyncDiffService, SyncWorkspace, ChangeRepository, SyncSelectionStore, OperationState, RefreshState, SourceControlViewModel, SourceControlActionService, and the ChangeRepository<->SyncStatusService wiring) out of main.ts and into src/runtime/createSyncRuntime.ts. main.ts now only knows Obsidian lifecycle: settings load/save, view/command/ribbon registration, vault event registration -- it no longer needs to know the sync/Source Control constructor graph to add a plugin lifecycle hook. Co-Authored-By: Claude Sonnet 5 --- src/main.ts | 111 +++++++------------ src/runtime/createSyncRuntime.ts | 140 ++++++++++++++++++++++++ tests/runtime/createSyncRuntime.test.ts | 88 +++++++++++++++ 3 files changed, 265 insertions(+), 74 deletions(-) create mode 100644 src/runtime/createSyncRuntime.ts create mode 100644 tests/runtime/createSyncRuntime.test.ts diff --git a/src/main.ts b/src/main.ts index 1ac9f5e..7e3658b 100644 --- a/src/main.ts +++ b/src/main.ts @@ -5,7 +5,7 @@ import { GitHubService } from './services/github-service'; import { GiteaService } from './services/gitea-service'; import { GitServiceInterface, GitTreeEntry } from './services/git-service-interface'; import { ConnectionTestResult } from './services/git-service-base'; -import { SyncManager } from './logic/sync-manager'; +import type { SyncManager } from './logic/sync-manager'; import { SourceControlItemView, SOURCE_CONTROL_VIEW_TYPE } from './ui/source-control/SourceControlItemView'; import { DiffTabView, SOURCE_CONTROL_DIFF_VIEW_TYPE, type DiffTabContent } from './ui/source-control/DiffTabView'; import { GitignoreManager } from './logic/gitignore-manager'; @@ -15,18 +15,16 @@ import { WhatsNewModal } from './ui/WhatsNewModal'; import { CHANGELOG, getUnseenReleases } from './changelog'; import { compareVersions } from './utils/version'; import { t, setLanguageOverride } from './i18n'; -import { ObsidianSyncInteraction } from './ui/ObsidianSyncInteraction'; -import { SyncStatusRefreshService } from './logic/sync/SyncStatusRefreshService'; -import { SyncDiffService } from './logic/sync/SyncDiffService'; -import { SyncManagerWorkspace, type SyncWorkspace } from './logic/sync/SyncWorkspace'; -import { ChangeRepository } from './logic/source-control/ChangeRepository'; -import { OperationState } from './logic/source-control/OperationState'; -import { RefreshState } from './logic/source-control/RefreshState'; -import { SyncSelectionStore } from './logic/source-control/SyncSelectionStore'; -import { SourceControlViewModel } from './logic/source-control/SourceControlViewModel'; -import { SourceControlActionService } from './logic/source-control/SourceControlActionService'; -import { SyncResultNotifier } from './logic/source-control/SyncResultNotifier'; -import { toSyncChanges } from './logic/source-control/FileStatusAdapter'; +import type { SyncStatusRefreshService } from './logic/sync/SyncStatusRefreshService'; +import type { SyncDiffService } from './logic/sync/SyncDiffService'; +import type { SyncWorkspace } from './logic/sync/SyncWorkspace'; +import type { ChangeRepository } from './logic/source-control/ChangeRepository'; +import type { OperationState } from './logic/source-control/OperationState'; +import type { RefreshState } from './logic/source-control/RefreshState'; +import type { SyncSelectionStore } from './logic/source-control/SyncSelectionStore'; +import type { SourceControlViewModel } from './logic/source-control/SourceControlViewModel'; +import type { SourceControlActionService } from './logic/source-control/SourceControlActionService'; +import { createSyncRuntime } from './runtime/createSyncRuntime'; import { filterFilesByVaultFolder as scopeFilterFiles, filterPathByVaultFolder as scopeFilterPath, @@ -55,7 +53,7 @@ export default class GitLabFilesPush extends Plugin { refreshState: RefreshState; sourceControlViewModel: SourceControlViewModel; sourceControlActions: SourceControlActionService; - private unsubscribeChangeRepository?: () => void; + private disposeSyncRuntime?: () => void; private gitignoreConfigKey = ''; private pushRibbonEl: HTMLElement; private statusBarEl: HTMLElement; @@ -92,68 +90,32 @@ export default class GitLabFilesPush extends Plugin { this.initializeGitService(); this.updateGitignoreManager(); - this.sync = new SyncManager( - this.app, - this.gitService, - this.settings, - this.saveSettings.bind(this), - (path) => this.gitignoreManager.isIgnored(this.getNormalizedPath(path)), - undefined, - new ObsidianSyncInteraction(this.app), - ); - this.syncStatusRefresh = new SyncStatusRefreshService({ + const runtime = createSyncRuntime({ app: this.app, - settings: () => this.settings, - gitService: () => this.gitService, - gitignoreManager: () => this.gitignoreManager, - syncManager: () => this.sync, + gitService: this.gitService, + getGitService: () => this.gitService, + settings: this.settings, + getSettings: () => this.settings, + saveSettings: this.saveSettings.bind(this), + getGitignoreManager: () => this.gitignoreManager, + isIgnored: (path) => this.gitignoreManager.isIgnored(this.getNormalizedPath(path)), filterFilesByVaultFolder: files => this.filterFilesByVaultFolder(files), filterPathByVaultFolder: path => this.filterPathByVaultFolder(path), getNormalizedPath: path => this.getNormalizedPath(path), getVaultPath: path => this.getVaultPath(path), - }, this.sync.status); - // One diff data service shared by the sync workspace (diff pane), - // the batch conflict modal's progressive +/- stat, and its "View - // Diff" — the modal never grows its own getBlob/cache path (see - // SyncDiffService.getConflictDiff). - this.syncDiffService = new SyncDiffService(this.sync.status, (sha, path) => this.gitService.getBlob(sha, path)); - this.sync.setConflictDiffStatLoader(conflict => this.syncDiffService.getConflictStat(conflict)); - this.sync.setConflictDiffLoader(conflict => this.syncDiffService.getConflictDiff(conflict)); - this.syncWorkspace = new SyncManagerWorkspace({ - manager: () => this.sync, - gitService: () => this.gitService, - settings: () => this.settings, - refreshService: this.syncStatusRefresh, - diffService: this.syncDiffService, - normalizePath: path => this.getNormalizedPath(path), - app: this.app, - }); - - this.changeRepository = new ChangeRepository(); - this.syncSelectionStore = new SyncSelectionStore(); - this.operationState = new OperationState(); - this.refreshState = new RefreshState(); - this.sourceControlViewModel = new SourceControlViewModel( - this.changeRepository, - this.syncSelectionStore, - this.operationState, - () => this.syncWorkspace.refresh(), - this.refreshState, - ); - this.sourceControlActions = new SourceControlActionService( - this.changeRepository, - this.operationState, - this.syncWorkspace, - new SyncResultNotifier(message => new Notice(message)), - ); - // Keeps ChangeRepository (and therefore the Source Control view) in - // sync with the same SyncStatusService instance the sync domain - // already publishes to -- no separate refresh/polling path. - this.unsubscribeChangeRepository = this.sync.status.subscribe((statuses) => { - const changes = toSyncChanges([...statuses.values()]); - this.changeRepository.replace(changes); - this.syncSelectionStore.refresh(changes.map(change => change.id)); + notify: message => new Notice(message), }); + this.sync = runtime.sync; + this.syncStatusRefresh = runtime.syncStatusRefresh; + this.syncDiffService = runtime.syncDiffService; + this.syncWorkspace = runtime.syncWorkspace; + this.changeRepository = runtime.changeRepository; + this.syncSelectionStore = runtime.syncSelectionStore; + this.operationState = runtime.operationState; + this.refreshState = runtime.refreshState; + this.sourceControlViewModel = runtime.sourceControlViewModel; + this.sourceControlActions = runtime.sourceControlActions; + this.disposeSyncRuntime = () => runtime.dispose(); this.statusBarEl = this.addStatusBarItem(); this.statusBarEl.addClass('gfs-status-bar-connection'); @@ -715,10 +677,11 @@ export default class GitLabFilesPush extends Plugin { onunload() { // Cleanup of registered components (views, commands, DOM/vault event - // listeners) is handled by Obsidian. The ChangeRepository subscription - // isn't Obsidian-managed, so it's unsubscribed explicitly. - this.unsubscribeChangeRepository?.(); - this.unsubscribeChangeRepository = undefined; + // listeners) is handled by Obsidian. The sync runtime's cross-object + // wiring (the ChangeRepository subscription) isn't Obsidian-managed, + // so it's disposed explicitly. + this.disposeSyncRuntime?.(); + this.disposeSyncRuntime = undefined; } async loadSettings() { diff --git a/src/runtime/createSyncRuntime.ts b/src/runtime/createSyncRuntime.ts new file mode 100644 index 0000000..8746f54 --- /dev/null +++ b/src/runtime/createSyncRuntime.ts @@ -0,0 +1,140 @@ +import type { App, TFile } from 'obsidian'; +import type { GitLabFilesPushSettings } from '../settings'; +import type { GitServiceInterface } from '../services/git-service-interface'; +import type { GitignoreManager } from '../logic/gitignore-manager'; +import { SyncManager } from '../logic/sync-manager'; +import { SyncStatusRefreshService } from '../logic/sync/SyncStatusRefreshService'; +import { SyncDiffService } from '../logic/sync/SyncDiffService'; +import { SyncManagerWorkspace, type SyncWorkspace } from '../logic/sync/SyncWorkspace'; +import { ChangeRepository } from '../logic/source-control/ChangeRepository'; +import { OperationState } from '../logic/source-control/OperationState'; +import { RefreshState } from '../logic/source-control/RefreshState'; +import { SyncSelectionStore } from '../logic/source-control/SyncSelectionStore'; +import { SourceControlViewModel } from '../logic/source-control/SourceControlViewModel'; +import { SourceControlActionService } from '../logic/source-control/SourceControlActionService'; +import { SyncResultNotifier } from '../logic/source-control/SyncResultNotifier'; +import { toSyncChanges } from '../logic/source-control/FileStatusAdapter'; +import { ObsidianSyncInteraction } from '../ui/ObsidianSyncInteraction'; + +export interface SyncRuntimeDependencies { + app: App; + /** The concrete git service in effect at construction time (SyncManager tracks changes via `updateGitService`). */ + gitService: GitServiceInterface; + getGitService: () => GitServiceInterface; + /** The settings object in effect at construction time (mutated in place, not replaced). */ + settings: GitLabFilesPushSettings; + getSettings: () => GitLabFilesPushSettings; + saveSettings: () => Promise; + getGitignoreManager: () => GitignoreManager; + isIgnored: (path: string) => boolean; + filterFilesByVaultFolder(files: TFile[]): TFile[]; + filterPathByVaultFolder(path: string): boolean; + getNormalizedPath(path: string): string; + getVaultPath(path: string): string; + notify: (message: string) => void; +} + +export interface SyncRuntime { + sync: SyncManager; + syncStatusRefresh: SyncStatusRefreshService; + syncDiffService: SyncDiffService; + syncWorkspace: SyncWorkspace; + changeRepository: ChangeRepository; + syncSelectionStore: SyncSelectionStore; + operationState: OperationState; + refreshState: RefreshState; + sourceControlViewModel: SourceControlViewModel; + sourceControlActions: SourceControlActionService; + /** Tears down cross-object wiring (the ChangeRepository subscription) that Obsidian does not manage. */ + dispose(): void; +} + +/** + * Wires the sync domain and Source Control application constructor graph + * together: SyncManager, SyncStatusRefreshService, SyncDiffService, + * SyncWorkspace, and the Source Control application layer built on top of + * it. Deliberately knows nothing about Obsidian lifecycle events, commands, + * views, or ribbons -- those stay owned by the plugin entry point. + */ +export function createSyncRuntime(deps: SyncRuntimeDependencies): SyncRuntime { + const sync = new SyncManager( + deps.app, + deps.gitService, + deps.settings, + deps.saveSettings, + deps.isIgnored, + undefined, + new ObsidianSyncInteraction(deps.app), + ); + + const syncStatusRefresh = new SyncStatusRefreshService({ + app: deps.app, + settings: deps.getSettings, + gitService: deps.getGitService, + gitignoreManager: deps.getGitignoreManager, + syncManager: () => sync, + filterFilesByVaultFolder: files => deps.filterFilesByVaultFolder(files), + filterPathByVaultFolder: path => deps.filterPathByVaultFolder(path), + getNormalizedPath: path => deps.getNormalizedPath(path), + getVaultPath: path => deps.getVaultPath(path), + }, sync.status); + + // One diff data service shared by the sync workspace (diff pane), the + // batch conflict modal's progressive +/- stat, and its "View Diff" -- the + // modal never grows its own getBlob/cache path (see + // SyncDiffService.getConflictDiff). + const syncDiffService = new SyncDiffService(sync.status, (sha, path) => deps.getGitService().getBlob(sha, path)); + sync.setConflictDiffStatLoader(conflict => syncDiffService.getConflictStat(conflict)); + sync.setConflictDiffLoader(conflict => syncDiffService.getConflictDiff(conflict)); + + const syncWorkspace = new SyncManagerWorkspace({ + manager: () => sync, + gitService: deps.getGitService, + settings: deps.getSettings, + refreshService: syncStatusRefresh, + diffService: syncDiffService, + normalizePath: path => deps.getNormalizedPath(path), + app: deps.app, + }); + + const changeRepository = new ChangeRepository(); + const syncSelectionStore = new SyncSelectionStore(); + const operationState = new OperationState(); + const refreshState = new RefreshState(); + const sourceControlViewModel = new SourceControlViewModel( + changeRepository, + syncSelectionStore, + operationState, + () => syncWorkspace.refresh(), + refreshState, + ); + const sourceControlActions = new SourceControlActionService( + changeRepository, + operationState, + syncWorkspace, + new SyncResultNotifier(deps.notify), + ); + + // Keeps ChangeRepository (and therefore the Source Control view) in sync + // with the same SyncStatusService instance the sync domain already + // publishes to -- no separate refresh/polling path. + const unsubscribeChangeRepository = sync.status.subscribe((statuses) => { + const changes = toSyncChanges([...statuses.values()]); + changeRepository.replace(changes); + syncSelectionStore.refresh(changes.map(change => change.id)); + }); + + return { + sync, + syncStatusRefresh, + syncDiffService, + syncWorkspace, + changeRepository, + syncSelectionStore, + operationState, + refreshState, + sourceControlViewModel, + sourceControlActions, + dispose: () => unsubscribeChangeRepository(), + }; +} diff --git a/tests/runtime/createSyncRuntime.test.ts b/tests/runtime/createSyncRuntime.test.ts new file mode 100644 index 0000000..41887c4 --- /dev/null +++ b/tests/runtime/createSyncRuntime.test.ts @@ -0,0 +1,88 @@ +import { describe, expect, it, vi } from 'vitest'; +import type { App, DataAdapter } from 'obsidian'; +import { createSyncRuntime } from '../../src/runtime/createSyncRuntime'; +import type { SyncRuntimeDependencies } from '../../src/runtime/createSyncRuntime'; +import type { GitLabFilesPushSettings } from '../../src/settings'; +import type { GitServiceInterface } from '../../src/services/git-service-interface'; + +vi.mock('obsidian'); + +function buildDeps(overrides: Partial = {}): SyncRuntimeDependencies { + const mockAdapter = { list: vi.fn().mockResolvedValue({ files: [], folders: [] }) } as unknown as DataAdapter; + const mockApp = { + vault: { + getFiles: () => [], + adapter: mockAdapter, + }, + } as unknown as App; + const mockGitService = { + listFilesDetailed: vi.fn().mockResolvedValue([]), + getBranchHead: vi.fn().mockResolvedValue(undefined), + } as unknown as GitServiceInterface; + const mockSettings = { + serviceType: 'github', + branch: 'main', + syncMetadata: {}, + vaultFolder: '', + rootPath: '', + } as unknown as GitLabFilesPushSettings; + const mockGitignoreManager = { isIgnored: () => false, loadGitignores: vi.fn().mockResolvedValue(undefined) } as never; + + return { + app: mockApp, + gitService: mockGitService, + getGitService: () => mockGitService, + settings: mockSettings, + getSettings: () => mockSettings, + saveSettings: vi.fn().mockResolvedValue(undefined), + getGitignoreManager: () => mockGitignoreManager, + isIgnored: () => false, + filterFilesByVaultFolder: files => files, + filterPathByVaultFolder: () => true, + getNormalizedPath: path => path, + getVaultPath: path => path, + notify: vi.fn(), + ...overrides, + }; +} + +describe('createSyncRuntime', () => { + it('wires every collaborator on top of the same SyncManager/SyncStatusService pair', () => { + const runtime = createSyncRuntime(buildDeps()); + + expect(runtime.sync).toBeDefined(); + expect(runtime.syncStatusRefresh).toBeDefined(); + expect(runtime.syncDiffService).toBeDefined(); + expect(runtime.syncWorkspace).toBeDefined(); + expect(runtime.changeRepository).toBeDefined(); + expect(runtime.syncSelectionStore).toBeDefined(); + expect(runtime.operationState).toBeDefined(); + expect(runtime.refreshState).toBeDefined(); + expect(runtime.sourceControlViewModel).toBeDefined(); + expect(runtime.sourceControlActions).toBeDefined(); + }); + + it('keeps ChangeRepository in sync with the shared SyncStatusService until disposed', () => { + const runtime = createSyncRuntime(buildDeps()); + + runtime.sync.status.set({ path: 'note.md', status: 'synced' }); + expect(runtime.changeRepository.getById('note.md' as never)).toBeDefined(); + + runtime.dispose(); + runtime.sync.status.set({ path: 'other.md', status: 'unsynced' }); + // Disposed: the second publish must not reach ChangeRepository. + expect(runtime.changeRepository.getById('other.md' as never)).toBeUndefined(); + }); + + it('routes SourceControlActionService notifications through the injected notify callback', async () => { + const notify = vi.fn(); + const runtime = createSyncRuntime(buildDeps({ notify })); + + // pull() with no matching changes resolves with an empty batch and no notification; + // this only asserts the runtime wired SourceControlActionService with our notifier, + // not any specific sync outcome. + await runtime.sourceControlActions.pull([]); + + expect(notify).not.toHaveBeenCalled(); + }); +}); From a4a1019fafd12a3358d33a805170530233925474 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 04:53:50 +0000 Subject: [PATCH 3/5] refactor(ui): extract SyncQueueSection and RepositoryChangesSection from SourceControlView Pull the "Sync Queue" and "Repository Changes" regions out of the 730-line SourceControlView into standalone render functions, matching the existing FilterMenu/SourceControlHeader/ChangeTree pattern: pure functions taking state + callbacks, never SyncWorkspace/SourceControlActionService/ SourceControlViewModel directly. SourceControlView keeps ownership of view state (collapsed sections, view mode, folder collapse) and now only orchestrates rendering, the diff pane, and scroll-state management. Co-Authored-By: Claude Sonnet 5 --- .../RepositoryChangesSection.ts | 99 ++++++++++ src/ui/source-control/SourceControlView.ts | 173 ++++-------------- src/ui/source-control/SyncQueueSection.ts | 92 ++++++++++ 3 files changed, 227 insertions(+), 137 deletions(-) create mode 100644 src/ui/source-control/RepositoryChangesSection.ts create mode 100644 src/ui/source-control/SyncQueueSection.ts diff --git a/src/ui/source-control/RepositoryChangesSection.ts b/src/ui/source-control/RepositoryChangesSection.ts new file mode 100644 index 0000000..1d3a679 --- /dev/null +++ b/src/ui/source-control/RepositoryChangesSection.ts @@ -0,0 +1,99 @@ +import { setIcon } from 'obsidian'; +import { t } from '../../i18n'; +import type { SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; +import { ICONS } from '../components/icons'; +import { renderChangeTree, renderChangeList, type ChangeTreeCallbacks } from './ChangeTree'; + +/** Tree shaping so the change tree stays a compact change view, not a full Explorer. */ +const TREE_OPTIONS = { collapseSingleChild: true }; +/** Mobile tree: collapse single-child folders and cap depth so the tree stays flat on a phone. */ +const MOBILE_TREE_OPTIONS = { collapseSingleChild: true, maxDepth: 2 }; + +export interface RepositoryChangesSectionState { + /** Rows not currently in the Sync Queue (the queue and this tree stay disjoint). */ + items: readonly SourceControlItem[]; + collapsed: boolean; + viewMode: 'tree' | 'list'; + collapsedFolders: Set; + isMobile: boolean; +} + +export interface RepositoryChangesSectionCallbacks { + onToggleCollapsed: () => void; + onSetViewMode: (mode: 'tree' | 'list') => void; +} + +/** + * Renders the "Repository Changes (N)" region: a collapsible header with a + * Tree/List view toggle, above the change tree/list itself. A single role + * label (not the active filter name — the filter chips above already carry + * that) makes the section's job — "navigate the source I can pick from" — + * distinct from the Sync Queue's "what I'm about to push". + * + * Pure presentation: receives only state and callbacks, never `SyncWorkspace`, + * `SourceControlActionService`, or `SourceControlViewModel` directly. + */ +export function renderRepositoryChangesSection( + container: HTMLElement, + state: RepositoryChangesSectionState, + treeCallbacks: ChangeTreeCallbacks, + sectionCallbacks: RepositoryChangesSectionCallbacks, +): void { + renderRepositoryHeader(container, state, sectionCallbacks); + if (state.collapsed) return; + + const treeWrap = container.createDiv({ cls: 'scv-changes-tree' }); + if (state.items.length === 0) { + treeWrap.createDiv({ cls: 'scv-empty', text: t('sourceControl.empty') }); + } else if (state.viewMode === 'list') { + renderChangeList(treeWrap, state.items, treeCallbacks); + } else { + renderChangeTree(treeWrap, state.items, state.collapsedFolders, treeCallbacks, state.isMobile ? MOBILE_TREE_OPTIONS : TREE_OPTIONS); + } +} + +/** + * The header collapses/expands the region; the Tree/List view toggle on the + * right stops propagation so switching presentation doesn't also collapse + * the section. + */ +function renderRepositoryHeader( + container: HTMLElement, + state: RepositoryChangesSectionState, + callbacks: RepositoryChangesSectionCallbacks, +): void { + const header = container.createDiv({ cls: 'scv-repository-header scv-collapsible-header' }); + header.setAttr('role', 'button'); + header.setAttr('aria-expanded', String(!state.collapsed)); + header.createSpan({ cls: 'scv-section-toggle', text: state.collapsed ? '▶' : '▼' }); + header.createSpan({ cls: 'scv-repository-title', text: t('sourceControl.section.repositoryChanges') }); + header.createSpan({ cls: 'scv-repository-count', text: String(state.items.length) }); + header.addEventListener('click', () => callbacks.onToggleCollapsed()); + renderViewToggle(header, state, callbacks); +} + +/** + * Tree/List segmented toggle, scoped to the Repository Changes region only + * (the Sync Queue is always a flat list, so it gets no such toggle). The + * active mode is highlighted; clicks stop propagation so they don't also + * collapse the section via the title area. + */ +function renderViewToggle( + container: HTMLElement, + state: RepositoryChangesSectionState, + callbacks: RepositoryChangesSectionCallbacks, +): void { + const toggle = container.createDiv({ cls: 'scv-view-toggle' }); + toggle.setAttr('role', 'group'); + toggle.setAttr('aria-label', t('sourceControl.view.toggleLabel')); + for (const mode of ['tree', 'list'] as const) { + const active = state.viewMode === mode; + const btn = toggle.createEl('button', { cls: `scv-view-toggle-btn${active ? ' is-active' : ''}` }); + btn.setAttr('data-view', mode); + btn.setAttr('aria-pressed', String(active)); + btn.setAttr('title', mode === 'tree' ? t('sourceControl.view.tree') : t('sourceControl.view.list')); + setIcon(btn.createSpan({ cls: 'scv-view-toggle-icon' }), mode === 'tree' ? ICONS.viewTree : ICONS.viewList); + btn.createSpan({ cls: 'scv-view-toggle-label', text: mode === 'tree' ? t('sourceControl.view.tree') : t('sourceControl.view.list') }); + btn.addEventListener('click', (evt) => { evt.stopPropagation(); callbacks.onSetViewMode(mode); }); + } +} diff --git a/src/ui/source-control/SourceControlView.ts b/src/ui/source-control/SourceControlView.ts index 8ef48a1..6c879af 100644 --- a/src/ui/source-control/SourceControlView.ts +++ b/src/ui/source-control/SourceControlView.ts @@ -7,11 +7,13 @@ import { defaultSyncAction, type SyncAction } from '../../logic/source-control/C import type { ChangeId } from '../../logic/source-control/types'; import { ICONS } from '../components/icons'; import { renderDiffViewer, currentDiffLayout, rememberDiffLayout, type DiffViewerHandle } from '../components/DiffViewer'; -import { renderChangeTree, renderChangeList, type ChangeTreeCallbacks } from './ChangeTree'; -import { renderChangeItem, type RowActionKind } from './ChangeItem'; +import type { ChangeTreeCallbacks } from './ChangeTree'; +import type { RowActionKind } from './ChangeItem'; import { DiffStatProvider, type DiffStatLoadResult } from './DiffStatProvider'; import { renderFilterMenu } from './FilterMenu'; import { renderSourceControlHeader, type SourceControlWorkspaceInfo } from './SourceControlHeader'; +import { renderSyncQueueSection } from './SyncQueueSection'; +import { renderRepositoryChangesSection } from './RepositoryChangesSection'; export interface SourceControlDiffContent { remote: string; @@ -85,11 +87,6 @@ export interface SourceControlViewCallbacks { loadDiffStat?: (item: SourceControlItem) => Promise; } -/** Tree shaping so the change tree stays a compact change view, not a full Explorer. */ -const TREE_OPTIONS = { collapseSingleChild: true }; -/** Mobile tree: collapse single-child folders and cap depth so the tree stays flat on a phone. */ -const MOBILE_TREE_OPTIONS = { collapseSingleChild: true, maxDepth: 2 }; - /** * Scroll positions of the main list's independently-scrolling regions, * persisted at View level so the mobile list → detail → Back round trip @@ -342,22 +339,42 @@ export class SourceControlView { // tree instead of blowing out the layout under // `.scv-root { overflow: hidden }`. const body = container.createDiv({ cls: 'scv-body' }); - this.renderSelectedSection(body, state.syncQueue, treeCallbacks); + renderSyncQueueSection( + body, + { + syncQueue: state.syncQueue, + collapsed: this.collapsedSections.has('checkedChanges'), + mobileCollapsed: this.mobileQueueCollapsed, + isMobile, + }, + treeCallbacks, + { + onToggleCollapsed: () => { + if (isMobile) { this.mobileQueueCollapsed = !this.mobileQueueCollapsed; this.rerender(); } + else this.toggleSection('checkedChanges'); + }, + onClearSelection: (items) => this.clearSelection(items), + }, + ); // The Changes region is its own flex/scroll area so a tall tree // scrolls independently and never pushes the pinned Sync Queue // region above it out of view. const changesRegion = body.createDiv({ cls: 'scv-changes-region' }); - this.renderRepositoryHeader(changesRegion, unchecked.length); - if (!this.collapsedSections.has('changes')) { - const treeWrap = changesRegion.createDiv({ cls: 'scv-changes-tree' }); - if (unchecked.length === 0) { - treeWrap.createDiv({ cls: 'scv-empty', text: t('sourceControl.empty') }); - } else if (this.viewMode === 'list') { - renderChangeList(treeWrap, unchecked, treeCallbacks); - } else { - renderChangeTree(treeWrap, unchecked, this.collapsedFolders, treeCallbacks, isMobile ? MOBILE_TREE_OPTIONS : TREE_OPTIONS); - } - } + renderRepositoryChangesSection( + changesRegion, + { + items: unchecked, + collapsed: this.collapsedSections.has('changes'), + viewMode: this.viewMode, + collapsedFolders: this.collapsedFolders, + isMobile, + }, + treeCallbacks, + { + onToggleCollapsed: () => this.toggleSection('changes'), + onSetViewMode: (mode) => this.setViewMode(mode), + }, + ); // Only rendered rows background-load their stats: a collapsed // Repository Changes section renders no tree, so hidden rows must // not fire provider fetches; expanding the section re-renders and @@ -422,49 +439,6 @@ export class SourceControlView { if (cursor !== null) newInput.setSelectionRange(cursor, cursor); } - /** - * Renders the "Repository Changes (N)" header above the change tree/list. - * A single role label (not the active filter name — the filter chips above - * already carry that) makes the section's job — "navigate the source I can - * pick from" — distinct from the Sync Queue's "what I'm about to push". - * The whole header collapses/expands the region; the Tree/List view toggle - * on the right stops propagation so switching presentation doesn't also - * collapse the section. - */ - private renderRepositoryHeader(container: HTMLElement, count: number): void { - const collapsed = this.collapsedSections.has('changes'); - const header = container.createDiv({ cls: 'scv-repository-header scv-collapsible-header' }); - header.setAttr('role', 'button'); - header.setAttr('aria-expanded', String(!collapsed)); - header.createSpan({ cls: 'scv-section-toggle', text: collapsed ? '▶' : '▼' }); - header.createSpan({ cls: 'scv-repository-title', text: t('sourceControl.section.repositoryChanges') }); - header.createSpan({ cls: 'scv-repository-count', text: String(count) }); - header.addEventListener('click', () => this.toggleSection('changes')); - this.renderViewToggle(header); - } - - /** - * Tree/List segmented toggle, scoped to the Repository Changes region only - * (the Sync Queue is always a flat list, so it gets no such toggle). The - * active mode is highlighted; clicks stop propagation so they don't also - * collapse the section via the title area. - */ - private renderViewToggle(container: HTMLElement): void { - const toggle = container.createDiv({ cls: 'scv-view-toggle' }); - toggle.setAttr('role', 'group'); - toggle.setAttr('aria-label', t('sourceControl.view.toggleLabel')); - for (const mode of ['tree', 'list'] as const) { - const active = this.viewMode === mode; - const btn = toggle.createEl('button', { cls: `scv-view-toggle-btn${active ? ' is-active' : ''}` }); - btn.setAttr('data-view', mode); - btn.setAttr('aria-pressed', String(active)); - btn.setAttr('title', mode === 'tree' ? t('sourceControl.view.tree') : t('sourceControl.view.list')); - setIcon(btn.createSpan({ cls: 'scv-view-toggle-icon' }), mode === 'tree' ? ICONS.viewTree : ICONS.viewList); - btn.createSpan({ cls: 'scv-view-toggle-label', text: mode === 'tree' ? t('sourceControl.view.tree') : t('sourceControl.view.list') }); - btn.addEventListener('click', (evt) => { evt.stopPropagation(); this.setViewMode(mode); }); - } - } - private setViewMode(mode: 'tree' | 'list'): void { if (this.viewMode === mode) return; this.viewMode = mode; @@ -477,75 +451,6 @@ export class SourceControlView { this.rerender(); } - /** - * Renders the "SYNC QUEUE" region — the working push batch, a flat list - * of the changes selected for sync. Each queued change is a normal change - * row (badge + name + diff-stat) with its selection checkbox checked: - * unchecking it here moves the row back down into the repository tree, - * and checking a repository row moves it up here, so the queue and the - * tree stay disjoint. The set comes straight from the ViewModel's - * single-source `syncQueue` projection (same definition as the Sync - * button count), so the section and the button can never drift. - * - * On mobile the queue renders expanded by default (same as desktop) so - * the upcoming changes are directly visible without an extra tap; the - * repository tree's own scroll region absorbs the height. Tapping the - * header collapses it to a header bar (the bottom sync bar still carries - * the count). - */ - private renderSelectedSection( - container: HTMLElement, - syncQueue: readonly SourceControlItem[], - callbacks: ChangeTreeCallbacks, - ): void { - if (syncQueue.length === 0) return; - const isMobile = Platform.isMobile; - const collapsed = isMobile ? this.mobileQueueCollapsed : this.collapsedSections.has('checkedChanges'); - const section = container.createDiv({ cls: 'scv-selected-section' }); - const header = section.createDiv({ cls: 'scv-selected-section-header scv-collapsible-header' }); - header.setAttr('role', 'button'); - header.setAttr('aria-expanded', String(!collapsed)); - header.createSpan({ cls: 'scv-section-toggle', text: collapsed ? '▶' : '▼' }); - header.createSpan({ cls: 'scv-selected-section-title', text: t('sourceControl.section.selectedForSync') }); - - const clearBtn = header.createEl('button', { - cls: 'scv-selected-section-clear', - attr: { type: 'button' }, - }); - clearBtn.createSpan({ cls: 'scv-selected-section-clear-label', text: t('sourceControl.section.clearSelection') }); - setTooltip(clearBtn, t('sourceControl.section.clearSelection.tooltip')); - clearBtn.addEventListener('click', (evt) => { evt.stopPropagation(); this.clearSelection(syncQueue); }); - header.addEventListener('click', () => { - if (isMobile) { this.mobileQueueCollapsed = !this.mobileQueueCollapsed; this.rerender(); } - else this.toggleSection('checkedChanges'); - }); - - if (collapsed) return; - section.createDiv({ - cls: 'scv-selected-section-subtitle', - text: t('sourceControl.section.queueSubtitle', { count: syncQueue.length }), - }); - const list = section.createDiv({ cls: 'scv-selected-section-list' }); - // Group the queue by its resolved sync action (the default, unless - // the user overrode it) so a mixed batch reads as what the Sync - // button will actually do (Upload / Download / Delete) rather than a - // flat list of ambiguous badges. Only surface group labels when more - // than one action is present in the batch — a single-action queue - // stays flat (no label noise) and matches the pre-categorization - // layout. - const upload = syncQueue.filter(item => item.syncAction === 'push'); - const download = syncQueue.filter(item => item.syncAction === 'pull'); - const deleteRemote = syncQueue.filter(item => item.syncAction === 'delete-remote'); - const groupCount = [upload, download, deleteRemote].filter(group => group.length > 0).length; - const mixed = groupCount > 1; - if (mixed && upload.length > 0) list.createDiv({ cls: 'scv-queue-group-label', text: t('sourceControl.queue.upload') }); - for (const item of upload) renderChangeItem(list, item, basename(item.path), callbacks, { showActionControl: true }); - if (mixed && download.length > 0) list.createDiv({ cls: 'scv-queue-group-label', text: t('sourceControl.queue.download') }); - for (const item of download) renderChangeItem(list, item, basename(item.path), callbacks, { showActionControl: true }); - if (mixed && deleteRemote.length > 0) list.createDiv({ cls: 'scv-queue-group-label', text: t('sourceControl.queue.delete') }); - for (const item of deleteRemote) renderChangeItem(list, item, basename(item.path), callbacks, { showActionControl: true }); - } - /** Unselects every change currently in the Sync Queue in one shot. */ private clearSelection(items: readonly SourceControlItem[]): void { this.viewModel.selection.deselectMany(items.map(item => item.id)); @@ -718,12 +623,6 @@ export class SourceControlView { } } -/** Last path segment of a change path, for the Selected section's flat row labels. */ -function basename(path: string): string { - const slash = path.lastIndexOf('/'); - return slash === -1 ? path : path.slice(slash + 1); -} - /** Attribute-safe escaping for a ChangeId used inside a `[data-change-id="…"]` selector. */ function escapeChangeId(id: string): string { if (typeof CSS !== 'undefined' && typeof CSS.escape === 'function') return CSS.escape(id); diff --git a/src/ui/source-control/SyncQueueSection.ts b/src/ui/source-control/SyncQueueSection.ts new file mode 100644 index 0000000..424ebf5 --- /dev/null +++ b/src/ui/source-control/SyncQueueSection.ts @@ -0,0 +1,92 @@ +import { setTooltip } from 'obsidian'; +import { t } from '../../i18n'; +import type { SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; +import { renderChangeItem } from './ChangeItem'; +import type { ChangeTreeCallbacks } from './ChangeTree'; + +export interface SyncQueueSectionState { + syncQueue: readonly SourceControlItem[]; + /** Desktop: the "Sync Queue" section's own collapse state. Mobile uses {@link mobileCollapsed} instead. */ + collapsed: boolean; + /** Mobile-only: the queue starts expanded by default, collapsed by tapping its header. */ + mobileCollapsed: boolean; + isMobile: boolean; +} + +export interface SyncQueueSectionCallbacks { + /** Toggles the section's collapse state — desktop `collapsedSections`, mobile `mobileQueueCollapsed`. */ + onToggleCollapsed: () => void; + /** Unselects every queued change in one shot. */ + onClearSelection: (items: readonly SourceControlItem[]) => void; +} + +/** + * Renders the "SYNC QUEUE" region — the working push batch, a flat list of + * the changes selected for sync. Each queued change is a normal change row + * (badge + name + diff-stat) with its selection checkbox checked: unchecking + * it here moves the row back down into the repository tree, and checking a + * repository row moves it up here, so the queue and the tree stay disjoint. + * + * On mobile the queue renders expanded by default (same as desktop) so the + * upcoming changes are directly visible without an extra tap; the repository + * tree's own scroll region absorbs the height. Tapping the header collapses + * it to a header bar (the bottom sync bar still carries the count). + * + * Pure presentation: receives only state and callbacks, never `SyncWorkspace`, + * `SourceControlActionService`, or `SourceControlViewModel` directly. + */ +export function renderSyncQueueSection( + container: HTMLElement, + state: SyncQueueSectionState, + treeCallbacks: ChangeTreeCallbacks, + sectionCallbacks: SyncQueueSectionCallbacks, +): void { + const { syncQueue } = state; + if (syncQueue.length === 0) return; + const collapsed = state.isMobile ? state.mobileCollapsed : state.collapsed; + const section = container.createDiv({ cls: 'scv-selected-section' }); + const header = section.createDiv({ cls: 'scv-selected-section-header scv-collapsible-header' }); + header.setAttr('role', 'button'); + header.setAttr('aria-expanded', String(!collapsed)); + header.createSpan({ cls: 'scv-section-toggle', text: collapsed ? '▶' : '▼' }); + header.createSpan({ cls: 'scv-selected-section-title', text: t('sourceControl.section.selectedForSync') }); + + const clearBtn = header.createEl('button', { + cls: 'scv-selected-section-clear', + attr: { type: 'button' }, + }); + clearBtn.createSpan({ cls: 'scv-selected-section-clear-label', text: t('sourceControl.section.clearSelection') }); + setTooltip(clearBtn, t('sourceControl.section.clearSelection.tooltip')); + clearBtn.addEventListener('click', (evt) => { evt.stopPropagation(); sectionCallbacks.onClearSelection(syncQueue); }); + header.addEventListener('click', () => sectionCallbacks.onToggleCollapsed()); + + if (collapsed) return; + section.createDiv({ + cls: 'scv-selected-section-subtitle', + text: t('sourceControl.section.queueSubtitle', { count: syncQueue.length }), + }); + const list = section.createDiv({ cls: 'scv-selected-section-list' }); + // Group the queue by its resolved sync action (the default, unless the + // user overrode it) so a mixed batch reads as what the Sync button will + // actually do (Upload / Download / Delete) rather than a flat list of + // ambiguous badges. Only surface group labels when more than one action + // is present in the batch — a single-action queue stays flat (no label + // noise) and matches the pre-categorization layout. + const upload = syncQueue.filter(item => item.syncAction === 'push'); + const download = syncQueue.filter(item => item.syncAction === 'pull'); + const deleteRemote = syncQueue.filter(item => item.syncAction === 'delete-remote'); + const groupCount = [upload, download, deleteRemote].filter(group => group.length > 0).length; + const mixed = groupCount > 1; + if (mixed && upload.length > 0) list.createDiv({ cls: 'scv-queue-group-label', text: t('sourceControl.queue.upload') }); + for (const item of upload) renderChangeItem(list, item, basename(item.path), treeCallbacks, { showActionControl: true }); + if (mixed && download.length > 0) list.createDiv({ cls: 'scv-queue-group-label', text: t('sourceControl.queue.download') }); + for (const item of download) renderChangeItem(list, item, basename(item.path), treeCallbacks, { showActionControl: true }); + if (mixed && deleteRemote.length > 0) list.createDiv({ cls: 'scv-queue-group-label', text: t('sourceControl.queue.delete') }); + for (const item of deleteRemote) renderChangeItem(list, item, basename(item.path), treeCallbacks, { showActionControl: true }); +} + +/** Last path segment of a change path, for the Sync Queue's flat row labels. */ +function basename(path: string): string { + const slash = path.lastIndexOf('/'); + return slash === -1 ? path : path.slice(slash + 1); +} From aa72da8d2da99e8fc85da376900db13e4c14d4ae Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 05:00:27 +0000 Subject: [PATCH 4/5] refactor(architecture): add ESLint boundary guards and fix a reverse sync->UI dependency Add no-restricted-imports rules per docs/architecture.md: src/ui/** and src/logic/source-control/** may not import a concrete Git provider or a push/pull coordinator/executor directly (must go through SyncWorkspace), and src/logic/sync/** may not import src/ui/source-control/** (dependency direction runs UI -> domain, never back). The last rule caught a real pre-existing violation: SyncDiffService and SyncInteractionPort (sync domain) imported computeDiffStat and DiffStatLoadResult from ui/source-control. Move the pure diff-stat computation (computeDiffStat, cheapLocalStat, addedContentStat, deletedContentStat) and the DiffStatLoadResult contract into a new src/logic/sync/DiffStat.ts; ChangePresentation.ts and DiffStatProvider.ts now depend on the domain for these instead of the other way around. Co-Authored-By: Claude Sonnet 5 --- eslint.config.mts | 69 ++++++++++++++++++ src/logic/sync/DiffStat.ts | 71 ++++++++++++++++++ src/logic/sync/SyncDiffService.ts | 3 +- src/logic/sync/SyncInteractionPort.ts | 2 +- src/ui/source-control/ChangeItem.ts | 3 +- src/ui/source-control/ChangePresentation.ts | 56 -------------- src/ui/source-control/DiffStatProvider.ts | 17 +---- .../source-control/SourceControlItemView.ts | 3 +- tests/logic/sync/DiffStat.test.ts | 73 +++++++++++++++++++ .../source-control/ChangePresentation.test.ts | 72 +----------------- .../source-control/DiffStatProvider.test.ts | 2 +- 11 files changed, 222 insertions(+), 149 deletions(-) create mode 100644 src/logic/sync/DiffStat.ts create mode 100644 tests/logic/sync/DiffStat.test.ts diff --git a/eslint.config.mts b/eslint.config.mts index f90dfb0..c529bbe 100644 --- a/eslint.config.mts +++ b/eslint.config.mts @@ -50,6 +50,75 @@ export default tseslint.config( ], }, }, + { + // Architecture regression guard (docs/architecture.md): the UI layer + // must reach the sync domain only through SyncWorkspace / the Source + // Control application services -- never a concrete Git provider or a + // push/pull coordinator/executor directly. + files: ["src/ui/**/*.ts", "src/ui/**/*.tsx"], + rules: { + "no-restricted-imports": [ + "error", + { + patterns: [ + { + group: ["**/services/github-service", "**/services/gitlab-service", "**/services/gitea-service"], + message: "UI must not depend on a concrete Git provider; go through SyncWorkspace instead.", + }, + { + group: ["**/logic/sync/PushCoordinator", "**/logic/sync/PullCoordinator", "**/logic/sync/PushExecutor", "**/logic/sync/PullExecutor"], + message: "UI must not bypass SyncWorkspace to reach a push/pull coordinator or executor directly.", + }, + ], + }, + ], + }, + }, + { + // Architecture regression guard (docs/architecture.md): the Source + // Control application layer (ChangeRepository, SourceControlActionService, + // SyncIntentExecutor, ...) must reach the sync domain only through + // SyncWorkspace -- never a concrete Git provider or a push/pull + // coordinator/executor directly. + files: ["src/logic/source-control/**/*.ts"], + rules: { + "no-restricted-imports": [ + "error", + { + patterns: [ + { + group: ["**/services/github-service", "**/services/gitlab-service", "**/services/gitea-service"], + message: "Source Control must not depend on a concrete Git provider; go through SyncWorkspace instead.", + }, + { + group: ["**/logic/sync/PushCoordinator", "**/logic/sync/PullCoordinator", "**/logic/sync/PushExecutor", "**/logic/sync/PullExecutor"], + message: "Source Control must not bypass SyncWorkspace to reach a push/pull coordinator or executor directly.", + }, + ], + }, + ], + }, + }, + { + // Architecture regression guard (docs/architecture.md): sync-domain + // modules (SyncManager, coordinators, executors, status resolution) + // must not depend on the Source Control presentation layer -- the + // dependency direction runs UI -> application -> domain, never back. + files: ["src/logic/sync/**/*.ts"], + rules: { + "no-restricted-imports": [ + "error", + { + patterns: [ + { + group: ["**/ui/source-control", "**/ui/source-control/*"], + message: "Sync-domain modules must not depend on the Source Control UI; the dependency direction runs UI -> domain, never back.", + }, + ], + }, + ], + }, + }, { files: ["src/**/*.ts", "src/**/*.tsx"], ...sonarjs.configs.recommended, diff --git a/src/logic/sync/DiffStat.ts b/src/logic/sync/DiffStat.ts new file mode 100644 index 0000000..061907a --- /dev/null +++ b/src/logic/sync/DiffStat.ts @@ -0,0 +1,71 @@ +import { computeSideBySideDiff } from '../../utils/diff'; + +/** Additions/deletions for a single change's diff, the +/- stat a row shows. */ +export interface ChangeStat { + additions: number; + deletions: number; +} + +/** + * What a diff-stat load resolved to for one row. The distinction matters + * because a cache treats the three outcomes differently: + * - `ready` — cached as a usable stat. + * - `unavailable` — permanent (binary, symlink, no two sides to diff); + * cached so the row is never retried. + * - `pending` — the backing content simply isn't in memory yet (e.g. a + * `local-only` row whose `localContent` hasn't been read). NOT cached: + * the next load pass retries the row, so a late-arriving stat still lands. + */ +export type DiffStatLoadResult = + | { status: 'ready'; stat: ChangeStat } + | { status: 'pending' } + | { status: 'unavailable' }; + +/** + * +/- stat for a two-sided diff (local-modified / remote-only / + * remote-modified / moved / conflict), reusing the existing LCS op logic in + * `utils/diff.ts`. Additions = added ops, deletions = removed ops. + */ +export function computeDiffStat(remote: string, local: string): ChangeStat { + const rows = computeSideBySideDiff(remote, local); + let additions = 0; + let deletions = 0; + for (const row of rows) { + if (row.right.type === 'added') additions++; + if (row.left.type === 'removed') deletions++; + } + return { additions, deletions }; +} + +/** + * Cheap stat for a `local-only` change: additions only (the local line + * count), no deletions and no remote/provider call. A trailing newline + * doesn't add a phantom line. + */ +export function cheapLocalStat(local: string): ChangeStat { + return { additions: countLines(local), deletions: 0 }; +} + +/** + * Stat for a one-sided change whose only content is the ADDED side: every + * line is an addition, no deletions. Used for `local-only` (A) and + * `remote-only` (↓) — both show +N, not the -N a content-vs-'' diff would + * produce for the download direction. + */ +export function addedContentStat(content: string): ChangeStat { + return { additions: countLines(content), deletions: 0 }; +} + +/** + * Stat for a one-sided DELETION: the content existed remotely and is gone + * locally, so every line is a deletion. Used for `local-deleted` (D). + */ +export function deletedContentStat(content: string): ChangeStat { + return { additions: 0, deletions: countLines(content) }; +} + +function countLines(s: string): number { + if (s === '') return 0; + const lines = s.replace(/\r\n/g, '\n').replace(/\r/g, '\n').split('\n'); + return lines[lines.length - 1] === '' ? lines.length - 1 : lines.length; +} diff --git a/src/logic/sync/SyncDiffService.ts b/src/logic/sync/SyncDiffService.ts index 27e1487..d6a6f1c 100644 --- a/src/logic/sync/SyncDiffService.ts +++ b/src/logic/sync/SyncDiffService.ts @@ -1,8 +1,7 @@ import type { SyncStatusService } from '../sync-status-service'; import { isBinaryPath } from '../../utils/path'; import type { FileDiff } from './types'; -import { computeDiffStat } from '../../ui/source-control/ChangePresentation'; -import type { DiffStatLoadResult } from '../../ui/source-control/DiffStatProvider'; +import { computeDiffStat, type DiffStatLoadResult } from './DiffStat'; export type BlobReader = (sha: string, path: string) => Promise<{ content: string | ArrayBuffer }>; diff --git a/src/logic/sync/SyncInteractionPort.ts b/src/logic/sync/SyncInteractionPort.ts index 1baeaec..021f352 100644 --- a/src/logic/sync/SyncInteractionPort.ts +++ b/src/logic/sync/SyncInteractionPort.ts @@ -1,4 +1,4 @@ -import type { DiffStatLoadResult } from '../../ui/source-control/DiffStatProvider'; +import type { DiffStatLoadResult } from './DiffStat'; import type { BatchPushConflict, SyncPlan } from './types'; export type SyncPlanDirection = 'push' | 'pull' | 'delete' | 'sync'; diff --git a/src/ui/source-control/ChangeItem.ts b/src/ui/source-control/ChangeItem.ts index a756c8d..1be944d 100644 --- a/src/ui/source-control/ChangeItem.ts +++ b/src/ui/source-control/ChangeItem.ts @@ -2,7 +2,8 @@ import { Menu, setIcon, setTooltip } from 'obsidian'; import { t, type TranslationKey } from '../../i18n'; import { ICONS } from '../components/icons'; import { renderOperationIndicator } from './OperationIndicator'; -import { presentChange, type ChangeStat } from './ChangePresentation'; +import { presentChange } from './ChangePresentation'; +import type { ChangeStat } from '../../logic/sync/DiffStat'; import { availableSyncActions, canDownload, type SyncAction } from '../../logic/source-control/ChangeActionPolicy'; import type { SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; import type { ChangeId, SyncChangeKind } from '../../logic/source-control/types'; diff --git a/src/ui/source-control/ChangePresentation.ts b/src/ui/source-control/ChangePresentation.ts index b051abe..c94ad1d 100644 --- a/src/ui/source-control/ChangePresentation.ts +++ b/src/ui/source-control/ChangePresentation.ts @@ -1,14 +1,7 @@ -import { computeSideBySideDiff } from '../../utils/diff'; import { t, type TranslationKey } from '../../i18n'; import type { SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; import type { SyncChangeKind } from '../../logic/source-control/types'; -/** Additions/deletions for a single change's diff, the +/- stat a row shows. */ -export interface ChangeStat { - additions: number; - deletions: number; -} - /** * UI-only presentation of one change: the badge letter + class, a short * subtitle, the display name (with rename "from" separated out), and an @@ -74,53 +67,4 @@ export function presentChange(item: SourceControlItem, displayName: string): Cha if (item.kind === 'remote-only') view.tooltip = t('sourceControl.status.remoteAvailable.tooltip'); if (item.kind === 'local-deleted') view.tooltip = t('sourceControl.status.deletedLocally.tooltip'); return view; -} - -/** - * +/- stat for a two-sided diff (local-modified / remote-only / - * remote-modified / moved / conflict), reusing the existing LCS op logic in - * `utils/diff.ts`. Additions = added ops, deletions = removed ops. - */ -export function computeDiffStat(remote: string, local: string): ChangeStat { - const rows = computeSideBySideDiff(remote, local); - let additions = 0; - let deletions = 0; - for (const row of rows) { - if (row.right.type === 'added') additions++; - if (row.left.type === 'removed') deletions++; - } - return { additions, deletions }; -} - -/** - * Cheap stat for a `local-only` change: additions only (the local line - * count), no deletions and no remote/provider call. A trailing newline - * doesn't add a phantom line. - */ -export function cheapLocalStat(local: string): ChangeStat { - return { additions: countLines(local), deletions: 0 }; -} - -/** - * Stat for a one-sided change whose only content is the ADDED side: every - * line is an addition, no deletions. Used for `local-only` (A) and - * `remote-only` (↓) — both show +N, not the -N a content-vs-'' diff would - * produce for the download direction. - */ -export function addedContentStat(content: string): ChangeStat { - return { additions: countLines(content), deletions: 0 }; -} - -/** - * Stat for a one-sided DELETION: the content existed remotely and is gone - * locally, so every line is a deletion. Used for `local-deleted` (D). - */ -export function deletedContentStat(content: string): ChangeStat { - return { additions: 0, deletions: countLines(content) }; -} - -function countLines(s: string): number { - if (s === '') return 0; - const lines = s.replace(/\r\n/g, '\n').replace(/\r/g, '\n').split('\n'); - return lines[lines.length - 1] === '' ? lines.length - 1 : lines.length; } \ No newline at end of file diff --git a/src/ui/source-control/DiffStatProvider.ts b/src/ui/source-control/DiffStatProvider.ts index a5b0708..7b3dd78 100644 --- a/src/ui/source-control/DiffStatProvider.ts +++ b/src/ui/source-control/DiffStatProvider.ts @@ -1,19 +1,6 @@ -import type { ChangeStat } from './ChangePresentation'; +import type { ChangeStat, DiffStatLoadResult } from '../../logic/sync/DiffStat'; -/** - * What the loader resolved for one change row. The distinction matters - * because the cache treats the three outcomes differently: - * - `ready` — cached as a usable stat. - * - `unavailable` — permanent (binary, symlink, no two sides to diff); - * cached so the row is never retried. - * - `pending` — the backing content simply isn't in memory yet (e.g. a - * `local-only` row whose `localContent` hasn't been read). NOT cached: - * the next load pass retries the row, so a late-arriving stat still lands. - */ -export type DiffStatLoadResult = - | { status: 'ready'; stat: ChangeStat } - | { status: 'pending' } - | { status: 'unavailable' }; +export type { DiffStatLoadResult }; type DiffStatCacheEntry = | { state: 'ready'; stat: ChangeStat } diff --git a/src/ui/source-control/SourceControlItemView.ts b/src/ui/source-control/SourceControlItemView.ts index 4e9bf24..d20dbdb 100644 --- a/src/ui/source-control/SourceControlItemView.ts +++ b/src/ui/source-control/SourceControlItemView.ts @@ -7,8 +7,7 @@ import type { FileStatus } from '../../logic/sync-status-service'; import { toChangeId, type ChangeId } from '../../logic/source-control/types'; import { SourceControlView, type SourceControlViewCallbacks } from './SourceControlView'; import type { SourceControlWorkspaceInfo } from './SourceControlHeader'; -import { addedContentStat, cheapLocalStat, computeDiffStat, deletedContentStat } from './ChangePresentation'; -import type { DiffStatLoadResult } from './DiffStatProvider'; +import { addedContentStat, cheapLocalStat, computeDiffStat, deletedContentStat, type DiffStatLoadResult } from '../../logic/sync/DiffStat'; import { ConfirmModal } from '../ConfirmModal'; // Reuses the legacy sync-status view's registered type string so an already diff --git a/tests/logic/sync/DiffStat.test.ts b/tests/logic/sync/DiffStat.test.ts new file mode 100644 index 0000000..088dee6 --- /dev/null +++ b/tests/logic/sync/DiffStat.test.ts @@ -0,0 +1,73 @@ +import { describe, expect, it } from 'vitest'; +import { addedContentStat, cheapLocalStat, computeDiffStat, deletedContentStat } from '../../../src/logic/sync/DiffStat'; + +describe('computeDiffStat', () => { + it('counts additions and deletions from a two-sided diff', () => { + const remote = 'line1\nline2\nline3'; + const local = 'line1\nchanged\nline3\nline4'; + const stat = computeDiffStat(remote, local); + expect(stat.additions).toBe(2); + expect(stat.deletions).toBe(1); + }); + + it('reports zero for identical content', () => { + const stat = computeDiffStat('a\nb', 'a\nb'); + expect(stat).toEqual({ additions: 0, deletions: 0 }); + }); + + it('treats a pure addition as additions only', () => { + const stat = computeDiffStat('a', 'a\nb'); + expect(stat).toEqual({ additions: 1, deletions: 0 }); + }); + + it('treats a pure deletion as deletions only', () => { + const stat = computeDiffStat('a\nb', 'a'); + expect(stat).toEqual({ additions: 0, deletions: 1 }); + }); +}); + +describe('cheapLocalStat', () => { + it('counts local lines as additions with no deletions', () => { + expect(cheapLocalStat('a\nb\nc')).toEqual({ additions: 3, deletions: 0 }); + }); + + it('reports zero for empty content', () => { + expect(cheapLocalStat('')).toEqual({ additions: 0, deletions: 0 }); + }); + + it('does not count a trailing newline as a phantom line', () => { + expect(cheapLocalStat('a\nb\n')).toEqual({ additions: 2, deletions: 0 }); + }); + + it('normalizes CRLF line endings', () => { + expect(cheapLocalStat('a\r\nb\r\nc')).toEqual({ additions: 3, deletions: 0 }); + }); +}); + +describe('addedContentStat', () => { + it('counts every line as an addition for a one-sided +N change', () => { + expect(addedContentStat('line1\nline2')).toEqual({ additions: 2, deletions: 0 }); + }); + + it('reports zero for empty content', () => { + expect(addedContentStat('')).toEqual({ additions: 0, deletions: 0 }); + }); + + it('does not count a trailing newline as a phantom line', () => { + expect(addedContentStat('line1\nline2\n')).toEqual({ additions: 2, deletions: 0 }); + }); +}); + +describe('deletedContentStat', () => { + it('counts every line as a deletion for a one-sided -N change', () => { + expect(deletedContentStat('line1\nline2')).toEqual({ additions: 0, deletions: 2 }); + }); + + it('reports zero for empty content', () => { + expect(deletedContentStat('')).toEqual({ additions: 0, deletions: 0 }); + }); + + it('does not count a trailing newline as a phantom line', () => { + expect(deletedContentStat('line1\nline2\n')).toEqual({ additions: 0, deletions: 2 }); + }); +}); diff --git a/tests/ui/source-control/ChangePresentation.test.ts b/tests/ui/source-control/ChangePresentation.test.ts index 358f1ce..e8810d0 100644 --- a/tests/ui/source-control/ChangePresentation.test.ts +++ b/tests/ui/source-control/ChangePresentation.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, beforeAll } from 'vitest'; -import { addedContentStat, cheapLocalStat, computeDiffStat, deletedContentStat, presentChange } from '../../../src/ui/source-control/ChangePresentation'; +import { presentChange } from '../../../src/ui/source-control/ChangePresentation'; import type { SourceControlItem } from '../../../src/logic/source-control/SourceControlViewModel'; import { resolveSyncAction } from '../../../src/logic/source-control/ChangeActionPolicy'; import { toChangeId } from '../../../src/logic/source-control/types'; @@ -84,73 +84,3 @@ describe('presentChange', () => { expect(view.renameFrom).toBe('old.md'); }); }); - -describe('computeDiffStat', () => { - it('counts additions and deletions from a two-sided diff', () => { - const remote = 'line1\nline2\nline3'; - const local = 'line1\nchanged\nline3\nline4'; - const stat = computeDiffStat(remote, local); - expect(stat.additions).toBe(2); - expect(stat.deletions).toBe(1); - }); - - it('reports zero for identical content', () => { - const stat = computeDiffStat('a\nb', 'a\nb'); - expect(stat).toEqual({ additions: 0, deletions: 0 }); - }); - - it('treats a pure addition as additions only', () => { - const stat = computeDiffStat('a', 'a\nb'); - expect(stat).toEqual({ additions: 1, deletions: 0 }); - }); - - it('treats a pure deletion as deletions only', () => { - const stat = computeDiffStat('a\nb', 'a'); - expect(stat).toEqual({ additions: 0, deletions: 1 }); - }); -}); - -describe('cheapLocalStat', () => { - it('counts local lines as additions with no deletions', () => { - expect(cheapLocalStat('a\nb\nc')).toEqual({ additions: 3, deletions: 0 }); - }); - - it('reports zero for empty content', () => { - expect(cheapLocalStat('')).toEqual({ additions: 0, deletions: 0 }); - }); - - it('does not count a trailing newline as a phantom line', () => { - expect(cheapLocalStat('a\nb\n')).toEqual({ additions: 2, deletions: 0 }); - }); - - it('normalizes CRLF line endings', () => { - expect(cheapLocalStat('a\r\nb\r\nc')).toEqual({ additions: 3, deletions: 0 }); - }); -}); -describe('addedContentStat', () => { - it('counts every line as an addition for a one-sided +N change', () => { - expect(addedContentStat('line1\nline2')).toEqual({ additions: 2, deletions: 0 }); - }); - - it('reports zero for empty content', () => { - expect(addedContentStat('')).toEqual({ additions: 0, deletions: 0 }); - }); - - it('does not count a trailing newline as a phantom line', () => { - expect(addedContentStat('line1\nline2\n')).toEqual({ additions: 2, deletions: 0 }); - }); -}); - -describe('deletedContentStat', () => { - it('counts every line as a deletion for a one-sided -N change', () => { - expect(deletedContentStat('line1\nline2')).toEqual({ additions: 0, deletions: 2 }); - }); - - it('reports zero for empty content', () => { - expect(deletedContentStat('')).toEqual({ additions: 0, deletions: 0 }); - }); - - it('does not count a trailing newline as a phantom line', () => { - expect(deletedContentStat('line1\nline2\n')).toEqual({ additions: 0, deletions: 2 }); - }); -}); diff --git a/tests/ui/source-control/DiffStatProvider.test.ts b/tests/ui/source-control/DiffStatProvider.test.ts index 73e8938..be258cc 100644 --- a/tests/ui/source-control/DiffStatProvider.test.ts +++ b/tests/ui/source-control/DiffStatProvider.test.ts @@ -3,7 +3,7 @@ import { DiffStatProvider } from '../../../src/ui/source-control/DiffStatProvide import type { DiffStatLoadResult } from '../../../src/ui/source-control/DiffStatProvider'; import type { SourceControlItem } from '../../../src/logic/source-control/SourceControlViewModel'; import { resolveSyncAction } from '../../../src/logic/source-control/ChangeActionPolicy'; -import type { ChangeStat } from '../../../src/ui/source-control/ChangePresentation'; +import type { ChangeStat } from '../../../src/logic/sync/DiffStat'; import { toChangeId } from '../../../src/logic/source-control/types'; function item(id: string, kind: SourceControlItem['kind'] = 'local-only'): SourceControlItem { From 6780444dc76bcaf07efefcfee594ed8e7a08d432 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 05:03:13 +0000 Subject: [PATCH 5/5] docs: sync CLAUDE.md and architecture.md with the enforced module boundaries CLAUDE.md's "Code Architecture" section duplicated (and had drifted from) docs/architecture.md: it said GitLab/GitHub only (no Gitea, though GiteaService already existed) and described sync-manager.ts as a single file. Replace it with a short contract pointing at docs/architecture.md (and docs/bug-fix-guidelines.md for bug fixes) plus the two compatibility gotchas that aren't covered there. docs/architecture.md's module table and "Current hotspots" section now describe the merged code: createSyncRuntime as the composition root, SyncFileDiscovery/SyncStatusResolver/RenameReconciler as SyncStatusRefreshService's three collaborators, DiffStat.ts, and SourceControlView's reduced scope after the SyncQueueSection/ RepositoryChangesSection extraction. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 28 ++++++++++++++++++++-------- docs/architecture.md | 17 +++++++++++------ 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index d20bf5d..a59f202 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,15 +18,27 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co - Test: `npm run test` (runs vitest suite) - Version bump: `npm run version` (updates manifest.json and versions.json via script) -## Code Architecture -- **Type**: Obsidian Plugin (TypeScript) that syncs vault files with a GitLab or GitHub repository. -- **Entry Point**: `src/main.ts` contains the main `GitLabFilesPush` class extending `Plugin`. +## Architecture Contract + +Before production code changes: +- Read `docs/architecture.md`. +- For bug fixes also read `docs/bug-fix-guidelines.md`. +- Identify the owning module before editing. +- Preserve dependency direction. +- Do not bypass `SyncWorkspace`. +- Do not move domain/provider logic into UI. +- Do not duplicate status/conflict/rename/action policy. + +`docs/architecture.md` is the canonical module map (layers, ownership table, MUST/MUST NOT rules, current hotspots) — this file does not duplicate it. A short summary: + +- **Type**: Obsidian Plugin (TypeScript) that syncs vault files with a GitHub, GitLab, or Gitea repository. +- **Entry point**: `src/main.ts` owns only Obsidian lifecycle (settings load/save, command/view/ribbon/vault-event registration); the sync/Source Control constructor graph is wired by `src/runtime/createSyncRuntime.ts`. - **Settings**: `src/settings.ts` defines `GitLabFilesPushSettings` interface, `DEFAULT_SETTINGS` object, and `GitLabSyncSettingTab` for the Obsidian UI. -- **Services**: `src/services/` abstracts the git provider behind `GitServiceInterface`, with `GitHubService` and `GitLabService` implementations sharing common logic via `BaseGitService`. -- **Sync logic**: `src/logic/sync-manager.ts` handles push/pull, conflict detection, and rename detection; `src/logic/gitignore-manager.ts` merges local and remote `.gitignore` rules. -- **UI**: the production Source Control surface is `SourceControlItemView` (`src/ui/source-control/SourceControlItemView.ts`), which renders `SourceControlView` (`src/ui/source-control/SourceControlView.ts`). User intent (push/pull/delete-remote/resolve-conflict) flows through `SourceControlActionService` (`src/logic/source-control/SourceControlActionService.ts`) into `SyncWorkspace` (`src/logic/sync/SyncWorkspace.ts`), which drives `SyncManager` and its executors (`PushExecutor`, `PullExecutor`, `RemoteDeleteExecutor`, etc. in `src/logic/sync/`). `src/ui/components/` holds shared diff/change presentation pieces used by this surface. - - Do not reintroduce `SyncStatusView` or `ui/sync-status/*` — that legacy presentation layer was replaced by the Source Control surface above and is blocked by an ESLint `no-restricted-imports` rule (`eslint.config.*`). The historical migration docs live in `docs/source-control-refactor/` and are marked as such; they are not current implementation guidance. - - `SOURCE_CONTROL_VIEW_TYPE` (`'sync-status-view'`) and the `open-sync-status` command id are intentionally kept as-is for pinned-leaf/workspace-layout compatibility — they resolve to the current `SourceControlItemView`, not a leftover of the old UI. Do not rename them as "cleanup." + +Two compatibility gotchas not covered by `docs/architecture.md`: +- Do not reintroduce `SyncStatusView` or `ui/sync-status/*` — that legacy presentation layer was replaced by the Source Control surface (`SourceControlItemView`/`SourceControlView`) and is blocked by an ESLint `no-restricted-imports` rule (`eslint.config.*`). The historical migration docs live in `docs/source-control-refactor/` and are marked as such; they are not current implementation guidance. +- `SOURCE_CONTROL_VIEW_TYPE` (`'sync-status-view'`) and the `open-sync-status` command id are intentionally kept as-is for pinned-leaf/workspace-layout compatibility — they resolve to the current `SourceControlItemView`, not a leftover of the old UI. Do not rename them as "cleanup." + - **Bundling**: Uses `esbuild.config.mjs` for compilation from TypeScript to a single `main.js` file. - **Deployment**: Relies on `manifest.json` for plugin metadata and `versions.json` for version mapping/compatibility. diff --git a/docs/architecture.md b/docs/architecture.md index 73018be..bd5dcef 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -26,7 +26,8 @@ The dependency direction should normally flow downward. Results and state flow b | Layer | Module | Owns | Interacts with | Must not own | | --- | --- | --- | --- | --- | -| Plugin runtime | `src/main.ts` | Obsidian lifecycle, command/view/event registration, top-level wiring | settings, UI, sync runtime | sync planning, conflict algorithms, provider-specific workflow | +| Plugin runtime | `src/main.ts` | Obsidian lifecycle, command/view/event registration | settings, `createSyncRuntime`, UI | sync/Source Control constructor graph, sync planning, conflict algorithms, provider-specific workflow | +| Plugin runtime | `createSyncRuntime` (`src/runtime/createSyncRuntime.ts`) | wires `SyncManager`, `SyncStatusRefreshService`, `SyncDiffService`, `SyncWorkspace`, and the Source Control application layer together | the constructors it composes | Obsidian lifecycle events, commands, views, ribbons | | UI | `src/ui/source-control/*` | rendering, user interaction, Source Control composition | `SourceControlViewModel`, `SourceControlActionService` | provider API calls, sync classification rules | | Application | `ChangeRepository` | authoritative Source Control `SyncChange` snapshot | `FileStatusAdapter`, ViewModel, action services | remote Git or filesystem access | | Application | `SyncSelectionStore` | queued selection and explicit per-change action override | ViewModel, current repository snapshot | sync execution | @@ -43,10 +44,14 @@ The dependency direction should normally flow downward. Results and state flow b | Sync domain | `PushExecutor` | provider-side batch mutations | `GitServiceInterface`, metadata | UI state | | Sync domain | `PullExecutor` | local file application for pulls | Obsidian vault, metadata | Source Control UI state | | Sync domain | `RemoteDeleteExecutor` | remote deletion execution | `GitServiceInterface` | UI | -| Sync domain | `SyncStatusRefreshService` | local/remote discovery, status refresh, incremental file event reconciliation, out-of-band move reconciliation | vault, provider, gitignore, status store, metadata | Source Control rendering | +| Sync domain | `SyncStatusRefreshService` | orchestrates discovery → resolve → reconcile → publish, plus incremental create/modify/delete/rename event handling | `SyncFileDiscovery`, `SyncStatusResolver`, `RenameReconciler`, status store | Source Control rendering, the three algorithms below (delegates to their owning class) | +| Sync domain | `SyncFileDiscovery` | vault/hidden-file/remote-tree/gitignore/symlink discovery, remote-only vs local-deleted classification | vault, provider, gitignore, status store | status resolution, rename reconciliation | +| Sync domain | `SyncStatusResolver` | local-vs-remote status resolution: SHA/content comparison, baseline diff direction, `FileStatus` classification | provider, sync manager, status store | discovery, rename reconciliation | +| Sync domain | `RenameReconciler` | out-of-band (external) rename detection by orphan/candidate blob-sha matching | sync manager, status store | discovery, status resolution | | Sync domain | `SyncStatusService` | observable `FileStatus` store and status classification | refresh/sync domain, adapters | UI orchestration | | Sync domain | `SyncMetadataStore` | last-synced SHA and rename metadata persistence | manager/executors/coordinators | presentation | -| Sync domain | `SyncDiffService` | diff content/stat loading and cache | status store, blob loader, workspace | sync orchestration | +| Sync domain | `SyncDiffService` | diff content/stat loading and cache | status store, blob loader, workspace, `DiffStat` | sync orchestration | +| Sync domain | `DiffStat` (`src/logic/sync/DiffStat.ts`) | pure +/- diff-stat computation and the `DiffStatLoadResult` contract | diff utilities | UI rendering, provider calls | | Interaction boundary | `SyncInteractionPort` | domain-facing confirmation/conflict interaction contract | domain, Obsidian adapter | provider implementation | | UI adapter | `ObsidianSyncInteraction` | Obsidian modal/notice implementation of interaction port | `SyncInteractionPort`, modal UI | sync algorithms | | Infrastructure | `GitServiceInterface` | provider abstraction used by the sync domain | concrete provider services | UI/application state | @@ -157,9 +162,9 @@ If the owning module cannot fix a bug without crossing a forbidden boundary, imp Some current modules have high responsibility density. That is not permission to bypass them, and file size alone is not a reason to split them. -- `main.ts`: composition and Obsidian lifecycle are still concentrated here. -- `SyncStatusRefreshService`: currently owns several related refresh concerns (discovery, classification, incremental events, rename reconciliation). -- `SourceControlView`: currently contains substantial Source Control presentation/composition logic. +- `main.ts`: reduced to Obsidian lifecycle (settings load/save, command/view/ribbon/vault-event registration). The sync/Source Control constructor graph now lives in `createSyncRuntime` (`src/runtime/createSyncRuntime.ts`). +- `SyncStatusRefreshService`: reduced to orchestration (discovery → resolve → reconcile → publish) plus the incremental create/modify/delete/rename handlers. Discovery, status resolution, and rename reconciliation each now have a single owner: `SyncFileDiscovery`, `SyncStatusResolver`, `RenameReconciler`. +- `SourceControlView`: reduced by extracting the "Sync Queue" and "Repository Changes" regions into `SyncQueueSection`/`RepositoryChangesSection` (`src/ui/source-control/`, pure state+callbacks render functions matching `FilterMenu`/`SourceControlHeader`). Still owns the diff pane, scroll-state management, and section composition. - `PushCoordinator`: large, but still centered on one batch-push use case; split only when a stable responsibility boundary is identified. Future refactors should reduce these hotspots while preserving the dependency direction in this document.