From ab70ed8e044665bb53a7c9fa85163bbecf684f38 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 05:57:51 +0000 Subject: [PATCH 01/10] refactor(source-control): make view model projection-only SourceControlViewModel is now a pure read-only projection: it no longer exposes a `selection` getter or owns any subscription wiring. Selection mutation (select/deselect, batch select/deselect, action override set/clear) moves to SourceControlActionService, and the ChangeRepository -> SyncSelectionStore.reconcile() wiring moves to createSyncRuntime, which also drops the now-redundant explicit syncSelectionStore.refresh() call superseded by that reconciliation. SourceControlView calls injected callbacks instead of reaching into SyncSelectionStore directly. Co-Authored-By: Claude Sonnet 5 --- .../provider/suites/sync-manager.e2e.test.ts | 3 +- .../support/source-control-scenarios.ts | 2 +- .../support/two-client-sync-scenario.ts | 4 +- .../SourceControlActionService.ts | 50 +++++++++++++ .../source-control/SourceControlViewModel.ts | 19 ++--- src/runtime/createSyncRuntime.ts | 18 ++++- .../source-control/SourceControlItemView.ts | 5 ++ src/ui/source-control/SourceControlView.ts | 53 ++++++++------ .../SourceControlActionService.test.ts | 70 ++++++++++++++++++- .../SourceControlViewModel.test.ts | 23 +++--- tests/runtime/createSyncRuntime.test.ts | 37 ++++++++++ .../source-control/SourceControlView.test.ts | 39 +++++++++-- 12 files changed, 261 insertions(+), 62 deletions(-) diff --git a/e2e-tests/provider/suites/sync-manager.e2e.test.ts b/e2e-tests/provider/suites/sync-manager.e2e.test.ts index 718c068..88b591f 100644 --- a/e2e-tests/provider/suites/sync-manager.e2e.test.ts +++ b/e2e-tests/provider/suites/sync-manager.e2e.test.ts @@ -8,6 +8,7 @@ import { SyncManagerWorkspace } from '../../../src/logic/sync/SyncWorkspace'; import { SourceControlActionService } from '../../../src/logic/source-control/SourceControlActionService'; import { ChangeRepository } from '../../../src/logic/source-control/ChangeRepository'; import { OperationState } from '../../../src/logic/source-control/OperationState'; +import { SyncSelectionStore } from '../../../src/logic/source-control/SyncSelectionStore'; import { toChangeId } from '../../../src/logic/source-control/types'; // `import type` deliberately, not a value import: src/settings.ts also // exports settings-tab UI (GitLabSyncSettingTab -> FolderSuggest -> @@ -261,7 +262,7 @@ describe('SyncManager E2E', () => { normalizePath: p => p, app: {} as App, }); - const actionService = new SourceControlActionService(repository, operations, workspace); + const actionService = new SourceControlActionService(repository, new SyncSelectionStore(), operations, workspace); await actionService.deleteRemote([changeId]); diff --git a/e2e-tests/provider/support/source-control-scenarios.ts b/e2e-tests/provider/support/source-control-scenarios.ts index fdc6be7..cd24f76 100644 --- a/e2e-tests/provider/support/source-control-scenarios.ts +++ b/e2e-tests/provider/support/source-control-scenarios.ts @@ -211,7 +211,7 @@ export class SourceControlScenario { getDiff: (): Promise => Promise.resolve({ path: '', kind: 'text' } as FileDiff), }, ); - const actionService = new SourceControlActionService(repository, operations, workspace); + const actionService = new SourceControlActionService(repository, selection, operations, workspace); return { repository, selection, operations, actionService, workspace }; } } diff --git a/e2e-tests/provider/support/two-client-sync-scenario.ts b/e2e-tests/provider/support/two-client-sync-scenario.ts index 2470e54..49a8611 100644 --- a/e2e-tests/provider/support/two-client-sync-scenario.ts +++ b/e2e-tests/provider/support/two-client-sync-scenario.ts @@ -14,6 +14,7 @@ import { GitignoreManager } from '../../../src/logic/gitignore-manager'; import { ensureSyncWorkspaceRuntime } from '../../../src/logic/sync/SyncWorkspace'; import { ChangeRepository } from '../../../src/logic/source-control/ChangeRepository'; import { OperationState } from '../../../src/logic/source-control/OperationState'; +import { SyncSelectionStore } from '../../../src/logic/source-control/SyncSelectionStore'; import { SourceControlActionService } from '../../../src/logic/source-control/SourceControlActionService'; import { toSyncChanges } from '../../../src/logic/source-control/FileStatusAdapter'; import { @@ -67,6 +68,7 @@ export class TwoClient { */ private readonly statuses: SyncStatusService; private readonly repository = new ChangeRepository(); + private readonly selection = new SyncSelectionStore(); private readonly operations = new OperationState(); private readonly refreshService: SyncStatusRefreshService; private readonly actionService: SourceControlActionService; @@ -110,7 +112,7 @@ export class TwoClient { sync: this.manager, getNormalizedPath: path => path, }, this.statuses); - this.actionService = new SourceControlActionService(this.repository, this.operations, workspace); + this.actionService = new SourceControlActionService(this.repository, this.selection, this.operations, workspace); } // --- local vault ops -------------------------------------------------- diff --git a/src/logic/source-control/SourceControlActionService.ts b/src/logic/source-control/SourceControlActionService.ts index ec4788c..225d9ae 100644 --- a/src/logic/source-control/SourceControlActionService.ts +++ b/src/logic/source-control/SourceControlActionService.ts @@ -3,6 +3,8 @@ import { type SyncExecutionResult, type SyncResultNotificationPort } from './Syn import type { ChangeRepository } from './ChangeRepository'; import type { OperationState } from './OperationState'; import type { SourceControlItem } from './SourceControlViewModel'; +import type { SyncSelectionStore } from './SyncSelectionStore'; +import { defaultSyncAction, type SyncAction } from './ChangeActionPolicy'; import { SyncIntentExecutor } from './SyncIntentExecutor'; import type { SyncIntentRequest } from './SyncIntent'; import type { ChangeId, SyncChange } from './types'; @@ -29,12 +31,18 @@ export interface SourceControlDiffContent { * * Neither layer talks to a Git provider directly; SyncWorkspace remains the * execution boundary. + * + * Also owns the Sync Queue selection/action-override mutation boundary + * (select/deselect, set/clear a row's action override) on behalf of + * SyncSelectionStore, so the UI never reaches past this facade into that + * store directly. */ export class SourceControlActionService { private readonly syncIntentExecutor: SyncIntentExecutor; constructor( private readonly changes: ChangeRepository, + private readonly selection: SyncSelectionStore, private readonly operations: OperationState, private readonly workspace: SyncWorkspace, private readonly syncResultNotifier: SyncResultNotificationPort = { notify: () => {} }, @@ -47,6 +55,48 @@ export class SourceControlActionService { ); } + /** Adds one change to the Sync Queue. */ + selectForSync(changeId: ChangeId): void { + this.selection.selectForSync(changeId); + } + + /** Removes one change from the Sync Queue, clearing any action override with it. */ + deselectFromSync(changeId: ChangeId): void { + this.selection.deselectFromSync(changeId); + } + + /** Adds several changes to the Sync Queue in one batch (e.g. a folder checkbox). */ + selectMany(changeIds: readonly ChangeId[]): void { + this.selection.selectMany(changeIds); + } + + /** Removes several changes from the Sync Queue in one batch. */ + deselectMany(changeIds: readonly ChangeId[]): void { + this.selection.deselectMany(changeIds); + } + + /** + * Sets a Sync Queue row's explicit action override. Picking the kind's + * own default clears the override instead of storing a redundant one, so + * `SourceControlItem.hasActionOverride` only means "the user chose + * something other than the default". + */ + setSyncAction(changeId: ChangeId, action: SyncAction): void { + const change = this.changes.getById(changeId); + if (!change) return; + + if (action === defaultSyncAction(change.kind)) { + this.selection.clearActionOverride(changeId); + } else { + this.selection.setActionOverride(changeId, action); + } + } + + /** Clears a Sync Queue row's explicit action override, reverting it to the kind default. */ + clearSyncAction(changeId: ChangeId): void { + this.selection.clearActionOverride(changeId); + } + /** Pushes one or more changes (single push and batch push share this path). */ async push(changeIds: readonly ChangeId[]): Promise { const targets = this.resolve(changeIds); diff --git a/src/logic/source-control/SourceControlViewModel.ts b/src/logic/source-control/SourceControlViewModel.ts index 849581d..96c1baa 100644 --- a/src/logic/source-control/SourceControlViewModel.ts +++ b/src/logic/source-control/SourceControlViewModel.ts @@ -35,10 +35,11 @@ export interface SourceControlViewState { * Read-only projection of repository, selection, operation, and refresh state * into UI-ready snapshots. * - * The constructor wires selection-intent reconciliation to authoritative - * ChangeRepository replacements. Cleanup therefore happens on the write-side - * repository lifecycle, while repeated getState() calls remain observational - * and never mutate queue intent. + * Purely observational: getState() never mutates queue intent, and this + * class exposes no selection mutation surface of its own. Selection-intent + * reconciliation against authoritative ChangeRepository replacements is + * wired by the runtime composition root (createSyncRuntime), not here, and + * mutation goes through SourceControlActionService instead of this class. */ export class SourceControlViewModel { constructor( @@ -47,15 +48,7 @@ export class SourceControlViewModel { private readonly operations: OperationState, private readonly refreshSource: () => Promise, private readonly refreshState: RefreshState, - ) { - this.changes.subscribe(changes => this.selectionStore.reconcile(changes)); - } - - /** - * Existing UI mutation boundary for queue selection. Kept for this PR to - * avoid mixing a renderer API redesign into the intent/execution cleanup. - */ - get selection(): SyncSelectionStore { return this.selectionStore; } + ) {} getState(filter: SourceControlFilter = 'all', showSynced = false): SourceControlViewState { const all = this.changes.getAll(); diff --git a/src/runtime/createSyncRuntime.ts b/src/runtime/createSyncRuntime.ts index 8746f54..f696592 100644 --- a/src/runtime/createSyncRuntime.ts +++ b/src/runtime/createSyncRuntime.ts @@ -110,18 +110,27 @@ export function createSyncRuntime(deps: SyncRuntimeDependencies): SyncRuntime { ); const sourceControlActions = new SourceControlActionService( changeRepository, + syncSelectionStore, operationState, syncWorkspace, new SyncResultNotifier(deps.notify), ); + // Selection-intent reconciliation is wired here, at the composition + // root, rather than inside SourceControlViewModel: it is a write-side + // lifecycle concern (stale selections/overrides get dropped whenever the + // repository publishes an authoritative snapshot), not part of the + // ViewModel's read-only projection. + const unsubscribeSelectionReconciliation = changeRepository.subscribe(changes => syncSelectionStore.reconcile(changes)); + // 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. + // publishes to -- no separate refresh/polling path. SyncSelectionStore + // cleanup is handled by the reconciliation subscription above, which + // ChangeRepository.replace() below triggers, so it isn't repeated here. const unsubscribeChangeRepository = sync.status.subscribe((statuses) => { const changes = toSyncChanges([...statuses.values()]); changeRepository.replace(changes); - syncSelectionStore.refresh(changes.map(change => change.id)); }); return { @@ -135,6 +144,9 @@ export function createSyncRuntime(deps: SyncRuntimeDependencies): SyncRuntime { refreshState, sourceControlViewModel, sourceControlActions, - dispose: () => unsubscribeChangeRepository(), + dispose: () => { + unsubscribeChangeRepository(); + unsubscribeSelectionReconciliation(); + }, }; } diff --git a/src/ui/source-control/SourceControlItemView.ts b/src/ui/source-control/SourceControlItemView.ts index d20dbdb..631d037 100644 --- a/src/ui/source-control/SourceControlItemView.ts +++ b/src/ui/source-control/SourceControlItemView.ts @@ -62,6 +62,11 @@ export class SourceControlItemView extends ItemView { onPush: (changeIds) => this.runAction(this.plugin.sourceControlActions.push(changeIds)), onDeleteRemote: (changeIds) => this.runAction(this.confirmThenDeleteRemote(changeIds)), onDeleteLocal: (changeIds) => this.runAction(this.plugin.sourceControlActions.deleteLocal(changeIds)), + onSelectForSync: (id) => this.plugin.sourceControlActions.selectForSync(id), + onDeselectFromSync: (id) => this.plugin.sourceControlActions.deselectFromSync(id), + onSelectMany: (ids) => this.plugin.sourceControlActions.selectMany(ids), + onDeselectMany: (ids) => this.plugin.sourceControlActions.deselectMany(ids), + onSetSyncAction: (id, action) => this.plugin.sourceControlActions.setSyncAction(id, action), }; this.view = new SourceControlView( this.plugin.sourceControlViewModel, diff --git a/src/ui/source-control/SourceControlView.ts b/src/ui/source-control/SourceControlView.ts index 6c879af..96f7821 100644 --- a/src/ui/source-control/SourceControlView.ts +++ b/src/ui/source-control/SourceControlView.ts @@ -3,7 +3,7 @@ import { t } from '../../i18n'; import type { SourceControlFilter } from '../../logic/source-control/SourceControlFilter'; import { SourceControlViewModel, type SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; import type { SyncIntentRequest } from '../../logic/source-control/SourceControlActionService'; -import { defaultSyncAction, type SyncAction } from '../../logic/source-control/ChangeActionPolicy'; +import type { SyncAction } from '../../logic/source-control/ChangeActionPolicy'; import type { ChangeId } from '../../logic/source-control/types'; import { ICONS } from '../components/icons'; import { renderDiffViewer, currentDiffLayout, rememberDiffLayout, type DiffViewerHandle } from '../components/DiffViewer'; @@ -56,6 +56,21 @@ export interface SourceControlViewCallbacks { onDeleteLocal?: (changeIds: ChangeId[]) => void | Promise; /** Triggers a view-wide refresh; the host wires this to the ViewModel's refresh delegate. */ onRefresh: () => void; + /** Adds one change to the Sync Queue — a Repository Changes row checkbox. */ + onSelectForSync: (id: ChangeId) => void; + /** Removes one change from the Sync Queue — a Sync Queue row checkbox. */ + onDeselectFromSync: (id: ChangeId) => void; + /** Adds several changes to the Sync Queue in one batch — a folder checkbox. */ + onSelectMany: (ids: readonly ChangeId[]) => void; + /** Removes several changes from the Sync Queue in one batch — a folder checkbox, or "Clear" on the queue. */ + onDeselectMany: (ids: readonly ChangeId[]) => void; + /** + * Records a Sync Queue row's explicit action override, chosen from its + * per-row action menu. Whether picking the kind's own default clears the + * override instead of storing it is decided behind this call + * (`SourceControlActionService.setSyncAction`), not by this view. + */ + onSetSyncAction: (id: ChangeId, action: SyncAction) => void; /** Notified when a change is selected for diff viewing, in addition to this view's own diff pane rendering. */ onOpenDiff?: (item: SourceControlItem) => void | Promise; /** Supplies diff content for the selected change; omit to leave the diff pane empty. */ @@ -103,12 +118,11 @@ interface MainScrollState { * from `SourceControlViewModel` state, per * docs/source-control-refactor/phase-3-source-control-ui.md. * - * Pure presentation + wiring: push/diff intent is handed to injected - * callbacks rather than acted on directly here, so this layer never reaches - * past the ViewModel to `SyncManager`/a Git provider. Selection toggling goes - * through `viewModel.selection` (the `SyncSelectionStore`, exposed by the - * ViewModel) so the view holds no selection reference of its own and the - * batch ops (`toggle`/`toggleMany`) live on the store, not inline here. + * Pure presentation + wiring: push/diff intent, and selection/action-override + * mutation alike, are handed to injected callbacks rather than acted on + * directly here, so this layer never reaches past the ViewModel/callbacks to + * `SyncManager`, a Git provider, or `SyncSelectionStore` — it holds no + * selection reference of its own. * * Rendering semantics (status-grouping fix): * - Every filter chip renders a single flat tree (or list). "All" composes @@ -453,7 +467,7 @@ export class SourceControlView { /** 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)); + this.callbacks.onDeselectMany(items.map(item => item.id)); this.rerender(); } @@ -493,18 +507,13 @@ export class SourceControlView { } /** - * Records (or clears) a Sync Queue row's explicit action override, chosen - * from its {@link ChangeItemCallbacks.onChangeSyncAction} menu. Picking - * the kind's own default clears the override rather than storing a - * redundant one, so `hasActionOverride` only ever means "the user chose - * something other than the default". + * Records a Sync Queue row's explicit action override, chosen from its + * {@link ChangeItemCallbacks.onChangeSyncAction} menu. Whether that + * clears a default-matching override instead of storing it is decided by + * `SourceControlActionService.setSyncAction`, not here. */ private changeSyncAction(item: SourceControlItem, action: SyncAction): void { - if (action === defaultSyncAction(item.kind)) { - this.viewModel.selection.clearActionOverride(item.id); - } else { - this.viewModel.selection.setActionOverride(item.id, action); - } + this.callbacks.onSetSyncAction(item.id, action); this.rerender(); } @@ -567,14 +576,14 @@ export class SourceControlView { } private toggleSelect(id: ChangeId, selected: boolean): void { - if (selected) this.viewModel.selection.selectForSync(id); - else this.viewModel.selection.deselectFromSync(id); + if (selected) this.callbacks.onSelectForSync(id); + else this.callbacks.onDeselectFromSync(id); this.rerender(); } private toggleFolderSelect(ids: readonly ChangeId[], selected: boolean): void { - if (selected) this.viewModel.selection.selectMany(ids); - else this.viewModel.selection.deselectMany(ids); + if (selected) this.callbacks.onSelectMany(ids); + else this.callbacks.onDeselectMany(ids); this.rerender(); } diff --git a/tests/logic/source-control/SourceControlActionService.test.ts b/tests/logic/source-control/SourceControlActionService.test.ts index dcabd59..60b8ac9 100644 --- a/tests/logic/source-control/SourceControlActionService.test.ts +++ b/tests/logic/source-control/SourceControlActionService.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from 'vitest'; import { ChangeRepository } from '../../../src/logic/source-control/ChangeRepository'; import { OperationState } from '../../../src/logic/source-control/OperationState'; +import { SyncSelectionStore } from '../../../src/logic/source-control/SyncSelectionStore'; import { SourceControlActionService, type SyncIntentRequest } from '../../../src/logic/source-control/SourceControlActionService'; import type { SyncExecutionResult, SyncResultNotificationPort } from '../../../src/logic/source-control/SyncResultNotifier'; import type { PlannedPushBatch } from '../../../src/logic/sync/PushCoordinator'; @@ -93,9 +94,10 @@ function buildService( ) { const repository = new ChangeRepository(); repository.replace(changes); + const selection = new SyncSelectionStore(); const operations = new OperationState(); - const service = new SourceControlActionService(repository, operations, workspace, notifier); - return { service, operations, notifier }; + const service = new SourceControlActionService(repository, selection, operations, workspace, notifier); + return { service, selection, operations, notifier }; } describe('SourceControlActionService', () => { @@ -798,4 +800,68 @@ describe('SourceControlActionService', () => { expect(content).toBeNull(); }); }); + + describe('selection mutation', () => { + it('selectForSync / deselectFromSync toggle one change through SyncSelectionStore', () => { + const { service, selection } = buildService( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }], + fakeWorkspace(), + ); + + service.selectForSync(toChangeId('c-1')); + expect(selection.isIncluded(toChangeId('c-1'))).toBe(true); + + service.deselectFromSync(toChangeId('c-1')); + expect(selection.isIncluded(toChangeId('c-1'))).toBe(false); + }); + + it('selectMany / deselectMany toggle a batch through SyncSelectionStore', () => { + const { service, selection } = buildService( + [ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }, + { id: toChangeId('c-2'), path: 'b.md', kind: 'local-only' }, + ], + fakeWorkspace(), + ); + + service.selectMany([toChangeId('c-1'), toChangeId('c-2')]); + expect(selection.getSelectedChangeIds()).toEqual([toChangeId('c-1'), toChangeId('c-2')]); + + service.deselectMany([toChangeId('c-1'), toChangeId('c-2')]); + expect(selection.getSelectedChangeIds()).toEqual([]); + }); + + it('setSyncAction stores a non-default override, and clears it once it matches the kind default', () => { + const { service, selection } = buildService( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }], + fakeWorkspace(), + ); + + service.setSyncAction(toChangeId('c-1'), 'pull'); + expect(selection.getActionOverride(toChangeId('c-1'))).toBe('pull'); + + // 'push' is local-modified's own default, so setting it back clears the override. + service.setSyncAction(toChangeId('c-1'), 'push'); + expect(selection.getActionOverride(toChangeId('c-1'))).toBeUndefined(); + }); + + it('clearSyncAction removes an explicit override', () => { + const { service, selection } = buildService( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }], + fakeWorkspace(), + ); + + selection.setActionOverride(toChangeId('c-1'), 'pull'); + service.clearSyncAction(toChangeId('c-1')); + + expect(selection.getActionOverride(toChangeId('c-1'))).toBeUndefined(); + }); + + it('setSyncAction on a stale (already-removed) change id is a no-op, not a throw', () => { + const { service, selection } = buildService([], fakeWorkspace()); + + expect(() => service.setSyncAction(toChangeId('gone'), 'pull')).not.toThrow(); + expect(selection.getActionOverride(toChangeId('gone'))).toBeUndefined(); + }); + }); }); diff --git a/tests/logic/source-control/SourceControlViewModel.test.ts b/tests/logic/source-control/SourceControlViewModel.test.ts index ff9c963..aa70ca2 100644 --- a/tests/logic/source-control/SourceControlViewModel.test.ts +++ b/tests/logic/source-control/SourceControlViewModel.test.ts @@ -217,22 +217,15 @@ describe('SourceControlViewModel', () => { expect(item?.hasActionOverride).toBe(true); }); - it('falls back to the default and clears a stale override once the kind no longer supports it', () => { - const repository = new ChangeRepository(); - repository.replace([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }]); - const selection = new SyncSelectionStore(); + it('falls back to the default once a stale override is no longer legal for the current kind', () => { + // Reconciling a stale override against a ChangeRepository replacement is + // wired by createSyncRuntime, not by SourceControlViewModel (see + // tests/runtime/createSyncRuntime.test.ts). This only verifies the + // ViewModel's own projection once SyncSelectionStore has already + // dropped the override. + const { viewModel, selection } = buildViewModel([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }]); selection.setActionOverride(toChangeId('c-1'), 'pull'); - const viewModel = new SourceControlViewModel( - repository, - selection, - new OperationState(), - vi.fn().mockResolvedValue(undefined), - new RefreshState(), - ); - - // Remote copy of the change disappears — kind moves from - // local-modified (allows pull) to local-only (push only). - repository.replace([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }]); + selection.reconcile([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }]); const item = viewModel.getState('all').items[0]; expect(item?.syncAction).toBe('push'); diff --git a/tests/runtime/createSyncRuntime.test.ts b/tests/runtime/createSyncRuntime.test.ts index 41887c4..f6eb8e3 100644 --- a/tests/runtime/createSyncRuntime.test.ts +++ b/tests/runtime/createSyncRuntime.test.ts @@ -74,6 +74,43 @@ describe('createSyncRuntime', () => { expect(runtime.changeRepository.getById('other.md' as never)).toBeUndefined(); }); + it('reconciles SyncSelectionStore against every ChangeRepository replacement, including stale overrides', () => { + const runtime = createSyncRuntime(buildDeps()); + + runtime.sync.status.set({ path: 'note.md', status: 'modified' }); + const noteId = runtime.changeRepository.getAll()[0]?.id; + expect(noteId).toBeDefined(); + if (!noteId) return; + + runtime.syncSelectionStore.selectForSync(noteId); + runtime.syncSelectionStore.setActionOverride(noteId, 'pull'); + expect(runtime.syncSelectionStore.isIncluded(noteId)).toBe(true); + + // Republishing without note.md at all drops the selection entirely. + runtime.sync.status.delete('note.md'); + + expect(runtime.syncSelectionStore.isIncluded(noteId)).toBe(false); + expect(runtime.syncSelectionStore.getActionOverride(noteId)).toBeUndefined(); + }); + + it('stops reconciling SyncSelectionStore once disposed', () => { + const runtime = createSyncRuntime(buildDeps()); + + runtime.sync.status.set({ path: 'note.md', status: 'modified' }); + const noteId = runtime.changeRepository.getAll()[0]?.id; + expect(noteId).toBeDefined(); + if (!noteId) return; + runtime.syncSelectionStore.selectForSync(noteId); + + runtime.dispose(); + // Calling ChangeRepository.replace() directly (bypassing sync.status) + // isolates the selection-reconciliation subscription specifically: + // after dispose(), it must no longer reach SyncSelectionStore. + runtime.changeRepository.replace([]); + + expect(runtime.syncSelectionStore.isIncluded(noteId)).toBe(true); + }); + it('routes SourceControlActionService notifications through the injected notify callback', async () => { const notify = vi.fn(); const runtime = createSyncRuntime(buildDeps({ notify })); diff --git a/tests/ui/source-control/SourceControlView.test.ts b/tests/ui/source-control/SourceControlView.test.ts index dd082b6..df0fa0e 100644 --- a/tests/ui/source-control/SourceControlView.test.ts +++ b/tests/ui/source-control/SourceControlView.test.ts @@ -6,11 +6,34 @@ import { OperationState } from '../../../src/logic/source-control/OperationState import { RefreshState } from '../../../src/logic/source-control/RefreshState'; import { SyncSelectionStore } from '../../../src/logic/source-control/SyncSelectionStore'; import { SourceControlViewModel, type SourceControlItem } from '../../../src/logic/source-control/SourceControlViewModel'; -import { toChangeId, type SyncChange } from '../../../src/logic/source-control/types'; +import { defaultSyncAction } from '../../../src/logic/source-control/ChangeActionPolicy'; +import { toChangeId, type ChangeId, type SyncChange } from '../../../src/logic/source-control/types'; import { setupObsidianDOM, createContainer } from '../setup-dom'; beforeAll(() => { setupObsidianDOM(); }); +/** + * Selection-mutation callbacks the view relies on, wired directly to a test + * SyncSelectionStore/ChangeRepository — a stand-in for what + * SourceControlActionService does against the real store in production. + */ +function selectionCallbacks( + repository: ChangeRepository, + selection: SyncSelectionStore, +): Pick { + return { + onSelectForSync: (id) => selection.selectForSync(id), + onDeselectFromSync: (id) => selection.deselectFromSync(id), + onSelectMany: (ids) => selection.selectMany(ids), + onDeselectMany: (ids) => selection.deselectMany(ids), + onSetSyncAction: (id: ChangeId, action) => { + const change = repository.getById(id); + if (change && action === defaultSyncAction(change.kind)) selection.clearActionOverride(id); + else selection.setActionOverride(id, action); + }, + }; +} + function buildView(changes: SyncChange[], callbacks: Partial = {}) { const repository = new ChangeRepository(); repository.replace(changes); @@ -21,7 +44,7 @@ function buildView(changes: SyncChange[], callbacks: Partial ({ + const view = new SourceControlView(viewModel, { onSync, onRefresh, ...selectionCallbacks(repository, selection), ...callbacks }, () => ({ serviceName: 'GitHub', branch: 'main', vaultFolder: '', @@ -41,7 +64,7 @@ function buildViewWithRepository(changes: SyncChange[], callbacks: Partial ({ + const view = new SourceControlView(viewModel, { onSync, onRefresh, ...selectionCallbacks(repository, selection), ...callbacks }, () => ({ serviceName: 'GitHub', branch: 'main', vaultFolder: '', @@ -1581,7 +1604,15 @@ describe('SourceControlView', () => { ); const view = new SourceControlView( viewModel, - { onSync: vi.fn(), onRefresh: vi.fn() }, + { + onSync: vi.fn(), + onRefresh: vi.fn(), + onSelectForSync: vi.fn(), + onDeselectFromSync: vi.fn(), + onSelectMany: vi.fn(), + onDeselectMany: vi.fn(), + onSetSyncAction: vi.fn(), + }, () => ({ serviceName: 'GitHub', branch: 'main', vaultFolder: '', lastCheckedAt }), ); return { view, refreshState }; From 2a206c7b6b21219a9abd1dc176a8524271fdd497 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 05:58:50 +0000 Subject: [PATCH 02/10] docs: record PR2 item 1 (source control state boundary) session progress Co-Authored-By: Claude Sonnet 5 --- progress.md | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/progress.md b/progress.md index 59226c3..d623494 100644 --- a/progress.md +++ b/progress.md @@ -5,14 +5,18 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont ## Current State **Last Updated:** 2026-09-01 -**Active Feature:** restore explicit per-file sync actions (no tracked issue number). All 7 planned commits landed and each individually passes lint/tests/build. -**Branch / PR:** `claude/fix-source-control-explicit-sync-intent`, based on `claude/fix-mobile-diff-rendering-and-responsive-layout` (itself 1 commit ahead of `main`). Not yet pushed or opened as a PR. +**Active Feature:** PR2 responsibility cleanup, item 1/5 — Source Control state boundary (no tracked issue number; an ad-hoc follow-up plan on top of `origin/1.6.1`, not in `feature_list.json`). +**Branch / PR:** `claude/pr2-source-control-boundary`, branched from `origin/1.6.1` (commit `69e5540`). Not yet pushed or opened as a PR. -**Scope:** `SyncSelectionStore`/`ChangeActionPolicy` (per-change action overrides + resolution), `SourceControlViewModel` (resolved `syncAction`/`hasActionOverride` projection), Sync Queue grouping/row controls, `SourceControlActionService.sync()` (now takes `SyncIntentRequest[]`), a new Repository Changes row "⋯" menu, and `SyncPlanModal` per-row direction icons. Deliberately did not touch `DiffViewer.ts`, mobile diff lifecycle, or E2E cleanup — that's the base branch's prior work. +**Scope (item 1 only, per the PR2 plan):** `SourceControlViewModel` is now a pure read-only projection — removed its `selection` getter and its constructor's `ChangeRepository.subscribe(... reconcile ...)` wiring. That reconciliation wiring now lives in `createSyncRuntime`, which also drops the redundant explicit `syncSelectionStore.refresh()` call it used to make alongside it (reconcile already supersedes it). Selection mutation (`selectForSync`/`deselectFromSync`/`selectMany`/`deselectMany`/`setSyncAction`/`clearSyncAction`) moved onto `SourceControlActionService`, which now also takes `SyncSelectionStore` in its constructor; `SourceControlView` calls injected callbacks instead of reaching into `SyncSelectionStore` via the ViewModel. Updated the 3 e2e-support call sites that constructed `SourceControlActionService` directly. Deliberately did not touch items 2-5 of the PR2 plan (Settings boundary, item-projection centralization, pull-orchestration reuse, provider contract cleanup) or any UX. -**Next:** push the branch and open the PR (title `fix(source-control): restore explicit per-file sync actions`); no further planned work outstanding. +**Next:** items 2-5 of the PR2 plan, one at a time, each its own commit — Settings boundary cleanup (item 2) is next up. -Below that: the previous "Outstanding Items"/"Verification Evidence" entries track separate, still-open work on PR #129 / `claude/source-control-foundation` and Issue #143 — not superseded by this entry, carried over from the base branch history. +Below that: the previous "Outstanding Items"/"Verification Evidence" entries track separate, still-open work on PR #129 / `claude/source-control-foundation`, Issue #143, and `claude/fix-source-control-explicit-sync-intent` — not superseded by this entry, carried over from the base branch history. + +- `npx eslint .` — 0 errors. +- `npx vitest run` — 74 files / 940 tests passed (up from 933; added SourceControlActionService selection-mutation tests and createSyncRuntime reconciliation-wiring tests). +- `npm run build` (tsc + Obsidian 1.11.0 compat typecheck + esbuild) — passed. ## Outstanding Items From 23a9e53d11fd3b3b3a4b19ead83e0613151ec1b8 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 06:32:38 +0000 Subject: [PATCH 03/10] refactor(settings): separate settings model from UI Splits the former src/settings-implementation.ts into src/settings/model.ts (SyncMetadata, GitServiceType, SymlinkHandling, GitLabFilesPushSettings, DEFAULT_SETTINGS), src/settings/helpers.ts (isSyncMetadataAtPath, getServiceName, getEffectiveSymlinkHandling), and src/ui/settings/GitLabSyncSettingTab.ts (all Obsidian Setting rendering). src/settings.ts becomes a thin public-compatibility re-export so every existing `from './settings'` import keeps working unchanged. The settings UI no longer imports the concrete GitLabFilesPush class for its own behavior: GitLabSyncSettingTab now depends on a narrow SettingsHost interface (settings, saveSettings, initializeGitService, testConnection, activateSourceControlView, onConnectionStatusChange). Its constructor takes `plugin: Plugin` and `host: SettingsHost` as separate parameters rather than an intersection type, since intersecting with Plugin re-introduces Plugin's own version-gated `settings` field and trips this repo's obsidianmd/no-unsupported-api lint rule. One remaining wart (RemoteFolderSuggest.attach still requires the concrete plugin class) is called out with a type-only cast and a comment rather than widened into SettingsHost or fixed here, since narrowing RemoteFolderSuggest itself is a separate, unrelated cleanup. No settings UX change; no lint config change. Co-Authored-By: Claude Sonnet 5 --- src/main.ts | 2 +- src/settings-implementation.ts | 588 ------------------ src/settings.ts | 11 +- src/settings/helpers.ts | 28 + src/settings/model.ts | 78 +++ src/ui/settings/GitLabSyncSettingTab.ts | 514 +++++++++++++++ tests/settings.test.ts | 58 ++ tests/ui/SettingsConnectionStatus.test.ts | 11 +- .../SettingsObsidian113Compatibility.test.ts | 6 +- 9 files changed, 698 insertions(+), 598 deletions(-) delete mode 100644 src/settings-implementation.ts create mode 100644 src/settings/helpers.ts create mode 100644 src/settings/model.ts create mode 100644 src/ui/settings/GitLabSyncSettingTab.ts create mode 100644 tests/settings.test.ts diff --git a/src/main.ts b/src/main.ts index 7e3658b..32ccc9f 100644 --- a/src/main.ts +++ b/src/main.ts @@ -64,7 +64,7 @@ export default class GitLabFilesPush extends Plugin { async onload() { await this.loadSettings(); - this.addSettingTab(new GitLabSyncSettingTab(this.app, this)); + this.addSettingTab(new GitLabSyncSettingTab(this.app, this, this)); this.registerView( SOURCE_CONTROL_VIEW_TYPE, diff --git a/src/settings-implementation.ts b/src/settings-implementation.ts deleted file mode 100644 index 96ab2d7..0000000 --- a/src/settings-implementation.ts +++ /dev/null @@ -1,588 +0,0 @@ -import {App, PluginSettingTab, Setting, Notice, TextComponent, ButtonComponent} from 'obsidian'; -import GitLabFilesPush, { type ConnectionStatus } from "./main"; -import {FolderSuggest} from "./ui/FolderSuggest"; -import {RemoteFolderSuggest} from "./ui/RemoteFolderSuggest"; -import {WhatsNewModal} from "./ui/WhatsNewModal"; -import { t, setLanguageOverride, type LanguageSetting } from "./i18n"; -import { CHANGELOG, entryText } from "./changelog"; - -// Minimal shape of Obsidian >= 1.13's SettingDefinitionItem. Declared locally so -// the plugin still type-checks against older Obsidian typings (minAppVersion -// 1.11.0), where this type does not exist. Obsidian only calls -// getSettingDefinitions() on versions that understand it. -interface SettingDefinitionItem { - name: string; - render: (setting: unknown, group: { listEl: HTMLElement }) => void; -} - -export interface SyncMetadata { - lastSyncedSha: string; - lastSyncedAt: number; - lastKnownPath?: string; - /** - * Set when the vault's 'rename' event moved this entry from another path - * and the move hasn't been pushed yet. Always the path still live on the - * remote — a chain of renames (A→B→C) collapses to this pointing at A, not - * the most recent hop, so pushing deletes the right remote path. - */ - renamedFrom?: string; -} - -/** - * Metadata written before `lastKnownPath` was introduced used its record key - * as the path. Keep that format eligible for rename reconciliation. - */ -export function isSyncMetadataAtPath(metadata: SyncMetadata | undefined, path: string): metadata is SyncMetadata { - return metadata !== undefined && (metadata.lastKnownPath === undefined || metadata.lastKnownPath === path); -} - -export type GitServiceType = 'gitlab' | 'github' | 'gitea'; - -/** - * How symbolic links (Git blobs with mode 120000) are synced: - * - 'real': recreate a real OS symlink on desktop; on mobile (no symlink API) - * fall back to syncing the link target's content as a normal file. - * - 'follow': always sync the target file's content as a normal file. - * - 'skip': ignore symlinks entirely. - */ -export type SymlinkHandling = 'real' | 'follow' | 'skip'; - -export interface GitLabFilesPushSettings { - serviceType: GitServiceType; - gitlabToken: string; - gitlabBaseUrl: string; - projectId: string; - githubToken: string; - githubOwner: string; - githubRepo: string; - giteaToken: string; - giteaBaseUrl: string; - giteaOwner: string; - giteaRepo: string; - branch: string; - syncMetadata: Record; - rootPath: string; - vaultFolder: string; - symlinkHandling: SymlinkHandling; - /** Multi-line, .gitignore-style patterns applied locally, in addition to the remote repo's .gitignore rules. */ - ignorePatterns: string; - /** Plugin version last seen by this vault, used to show a "what's new" tip after an update. */ - lastSeenVersion: string; - /** Version whose "what's new" banner in the settings tab has been dismissed, if any. */ - bannerDismissedVersion: string; - /** UI language. 'system' follows Obsidian's display language, falling back to English if unsupported. */ - language: LanguageSetting; - /** Refresh the sync status automatically after Obsidian finishes loading. */ - autoRefreshOnStartup: boolean; -} - -export function getServiceName(settings: GitLabFilesPushSettings): string { - if (settings.serviceType === 'gitlab') return 'GitLab'; - if (settings.serviceType === 'gitea') return 'Gitea'; - return 'GitHub'; -} - -/** - * Resolves the symlink behavior that actually applies. Only GitHub can create or - * push real symlinks (it has the Git Data API); on other providers "real" is not - * possible, so it is treated as "skip" to avoid silently turning links into - * ordinary files. - */ -export function getEffectiveSymlinkHandling(settings: GitLabFilesPushSettings): SymlinkHandling { - if (settings.symlinkHandling === 'real' && settings.serviceType !== 'github') { - return 'skip'; - } - return settings.symlinkHandling; -} - -export const DEFAULT_SETTINGS: GitLabFilesPushSettings = { - serviceType: 'gitlab', - gitlabToken: '', - gitlabBaseUrl: 'https://gitlab.com', - projectId: '', - githubToken: '', - githubOwner: '', - githubRepo: '', - giteaToken: '', - giteaBaseUrl: '', - giteaOwner: '', - giteaRepo: '', - rootPath: "", - branch: 'main', - syncMetadata: {}, - vaultFolder: '', - symlinkHandling: 'real', - ignorePatterns: '', - lastSeenVersion: '', - bannerDismissedVersion: '', - language: 'system', - autoRefreshOnStartup: true -} - -const CONNECTION_TEST_DEBOUNCE_MS = 800; - -export class GitLabSyncSettingTab extends PluginSettingTab { - plugin: GitLabFilesPush; - private statusBadgeEl: HTMLElement | null = null; - private connectionTestTimer: number | null = null; - private unsubscribeConnectionStatus: (() => void) | null = null; - - constructor(app: App, plugin: GitLabFilesPush) { - super(app, plugin); - this.plugin = plugin; - } - - // The status badge mirrors the plugin's shared connection status (also - // driving the status bar item) instead of running its own test, so both - // stay in sync and don't race separate requests against the remote API. - hide(): void { - this.unsubscribeConnectionStatus?.(); - this.unsubscribeConnectionStatus = null; - if (this.connectionTestTimer) { - window.clearTimeout(this.connectionTestTimer); - this.connectionTestTimer = null; - } - } - - // Kept as a fallback for Obsidian < 1.13.0 (older than 1.13, down to - // minAppVersion 1.11.0), which don't know about getSettingDefinitions() - // and always call display(). - display(): void { - this.renderSettings(this.containerEl); - } - - getSettingDefinitions(): SettingDefinitionItem[] { - return [{ - name: '', - render: (_setting, group) => { - this.renderSettings(group.listEl); - } - }]; - } - - private refresh(): void { - // update() only exists on Obsidian >= 1.13. On older versions (down to - // minAppVersion 1.11.0) re-render manually instead. Accessed via a cast - // so this compiles against the 1.11 typings, which lack update(). - const maybeUpdate = (this as { update?: () => void }).update; - if (typeof maybeUpdate === 'function') { - maybeUpdate.call(this); - } else { - this.renderSettings(this.containerEl); - } - } - - // Persistent (until dismissed) banner surfacing the current version's notable - // highlights right at the top of the settings tab. Dismissing this only hides - // the attention banner; release history remains available from Settings. - private renderWhatsNewBanner(containerEl: HTMLElement): void { - const currentVersion = this.plugin.manifest.version; - if (this.plugin.settings.bannerDismissedVersion === currentVersion) return; - - const release = CHANGELOG.find(r => r.version === currentVersion); - const notableEntries = release?.entries.filter(entry => entry.notable) ?? []; - if (notableEntries.length === 0) return; - - // Onboarding releases already teach their mental model in the modal's - // step-by-step layout — keep the banner itself to a couple of highlights - // rather than repeating every notable entry. - const bannerEntries = release?.onboarding ? notableEntries.slice(0, 2) : notableEntries; - - const banner = containerEl.createDiv({ cls: 'gfs-whats-new-banner' }); - const textEl = banner.createDiv({ cls: 'gfs-whats-new-banner-text' }); - textEl.createEl('strong', { text: t('settings.whatsNewBanner.title', { version: currentVersion }) }); - const list = textEl.createEl('ul', { cls: 'gfs-whats-new-banner-list' }); - for (const entry of bannerEntries) { - list.createEl('li', { text: entryText(entry) }); - } - const viewBtn = new ButtonComponent(textEl) - .setButtonText(t('settings.whatsNewBanner.view')) - .onClick(() => { - new WhatsNewModal(this.app, CHANGELOG, () => void this.plugin.activateSourceControlView()).open(); - }); - viewBtn.buttonEl.addClass('gfs-whats-new-banner-view'); - - const dismissBtn = banner.createEl('button', { - cls: 'gfs-whats-new-banner-dismiss', - text: '×', - attr: { 'aria-label': t('settings.whatsNewBanner.dismiss') } - }); - dismissBtn.addEventListener('click', () => { - void (async () => { - this.plugin.settings.bannerDismissedVersion = currentVersion; - await this.plugin.saveSettings(); - this.refresh(); - })(); - }); - } - - private renderReleaseHistorySetting(containerEl: HTMLElement): void { - new Setting(containerEl) - .setName(t('settings.releaseHistory.name')) - .setDesc(t('settings.releaseHistory.desc')) - .addButton(button => button - .setButtonText(t('settings.releaseHistory.button')) - .onClick(() => { - new WhatsNewModal(this.app, CHANGELOG, () => void this.plugin.activateSourceControlView()).open(); - })); - } - - // Rebuilding the whole settings tab (renderSettings) to refresh the badge - // would empty and recreate every field, stealing focus mid-typing. The - // badge element is instead created once per renderSettings pass and - // updated in place by setStatusBadge(), driven by the plugin's shared - // connection status (see main.ts) so it stays in sync with the status bar. - private renderConnectionStatus(containerEl: HTMLElement): void { - this.statusBadgeEl = containerEl.createDiv({ cls: 'gfs-connection-status' }); - this.unsubscribeConnectionStatus?.(); - this.unsubscribeConnectionStatus = this.plugin.onConnectionStatusChange((status) => this.setStatusBadge(status)); - } - - private setStatusBadge(status: ConnectionStatus): void { - const badge = this.statusBadgeEl; - if (!badge) return; - - badge.removeClass('is-checking', 'is-connected', 'is-disconnected'); - badge.addClass(`is-${status.state}`); - - const labels: Record = { - checking: t('settings.connectionStatus.checking'), - connected: t('settings.connectionStatus.connected'), - disconnected: t('settings.connectionStatus.disconnected') - }; - const label = labels[status.state]; - badge.setText(status.detail ? t('settings.connectionStatus.withDetail', { label, detail: status.detail }) : label); - } - - // Debounced so token/branch fields (which call this on every keystroke) - // don't hit the remote API on every character typed. - private scheduleConnectionTest(): void { - if (this.connectionTestTimer) { - window.clearTimeout(this.connectionTestTimer); - } - this.connectionTestTimer = window.setTimeout(() => { - this.connectionTestTimer = null; - void this.plugin.testConnection(); - }, CONNECTION_TEST_DEBOUNCE_MS); - } - - private renderSettings(containerEl: HTMLElement): void { - containerEl.empty(); - - this.renderWhatsNewBanner(containerEl); - this.renderReleaseHistorySetting(containerEl); - this.renderConnectionStatus(containerEl); - - new Setting(containerEl) - .setName(t('settings.language.name')) - .setDesc(t('settings.language.desc')) - .addDropdown(dropdown => dropdown - .addOption('system', t('settings.language.option.system')) - .addOption('en', t('settings.language.option.en')) - .addOption('zh-tw', t('settings.language.option.zhTw')) - .addOption('zh-cn', t('settings.language.option.zhCn')) - .setValue(this.plugin.settings.language) - .onChange((value: string) => { - this.plugin.settings.language = value as LanguageSetting; - void this.plugin.saveSettings(); - setLanguageOverride(this.plugin.settings.language); - this.refresh(); - })); - - new Setting(containerEl) - .setName(t('settings.gitService.name')) - .setDesc(t('settings.gitService.desc')) - .addDropdown(dropdown => dropdown - .addOption('gitlab', 'GitLab') - .addOption('github', 'GitHub') - .addOption('gitea', 'Gitea') - .setValue(this.plugin.settings.serviceType) - .onChange((value: string) => { - this.plugin.settings.serviceType = value as GitServiceType; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.refresh(); - })); - - new Setting(containerEl).setName('').setHeading(); - - if (this.plugin.settings.serviceType === 'gitlab') { - this.displayGitLabSettings(containerEl); - } else if (this.plugin.settings.serviceType === 'gitea') { - this.displayGiteaSettings(containerEl); - } else { - this.displayGitHubSettings(containerEl); - } - - new Setting(containerEl) - .setName(t('settings.branch.name')) - .setDesc(t('settings.branch.desc')) - .addText(text => text - .setPlaceholder(t('settings.branch.placeholder')) - .setValue(this.plugin.settings.branch) - .onChange((value) => { - this.plugin.settings.branch = value || 'main'; - void this.plugin.saveSettings(); - this.scheduleConnectionTest(); - })); - - new Setting(containerEl) - .setName(t('settings.rootPath.name')) - .setDesc(t('settings.rootPath.desc')) - .addText(text => { - text.setPlaceholder(t('settings.rootPath.placeholder')) - .setValue(this.plugin.settings.rootPath) - .onChange((value) => { - this.plugin.settings.rootPath = value.replace(/^\/|\/$/g, ''); - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - }); - RemoteFolderSuggest.attach(this.app, text.inputEl, this.plugin); - }); - - new Setting(containerEl) - .setName(t('settings.vaultFolder.name')) - .setDesc(t('settings.vaultFolder.desc')) - .addText(text => { - text.setPlaceholder(t('settings.vaultFolder.placeholder')) - .setValue(this.plugin.settings.vaultFolder) - .onChange((value) => { - this.plugin.settings.vaultFolder = value.replace(/^\/|\/$/g, ''); - void this.plugin.saveSettings(); - }); - FolderSuggest.attach(this.app, text.inputEl); - }); - - new Setting(containerEl) - .setName(t('settings.autoRefreshOnStartup.name')) - .setDesc(t('settings.autoRefreshOnStartup.desc')) - .addToggle(toggle => toggle - .setValue(this.plugin.settings.autoRefreshOnStartup) - .onChange((value) => { - this.plugin.settings.autoRefreshOnStartup = value; - void this.plugin.saveSettings(); - })); - - new Setting(containerEl) - .setName(t('settings.ignorePatterns.name')) - .setDesc(t('settings.ignorePatterns.desc')) - .addTextArea(text => { - text.setPlaceholder(`${this.app.vault.configDir}/\n*.tmp`) - .setValue(this.plugin.settings.ignorePatterns) - .onChange((value) => { - this.plugin.settings.ignorePatterns = value; - void this.plugin.saveSettings(); - }); - text.inputEl.rows = 4; - }); - - // "Real symlink" needs the Git Data API, which only GitHub offers. For - // other providers, offer follow/skip only so the option can't mislead. - const supportsRealSymlink = this.plugin.settings.serviceType === 'github'; - new Setting(containerEl) - .setName(t('settings.symlinks.name')) - .setDesc(supportsRealSymlink - ? t('settings.symlinks.desc.supported') - : t('settings.symlinks.desc.unsupported')) - .addDropdown(dropdown => { - if (supportsRealSymlink) dropdown.addOption('real', t('settings.symlinks.option.real')); - dropdown - .addOption('follow', t('settings.symlinks.option.follow')) - .addOption('skip', t('settings.symlinks.option.skip')) - .setValue(getEffectiveSymlinkHandling(this.plugin.settings)) - .onChange((value: string) => { - this.plugin.settings.symlinkHandling = value as SymlinkHandling; - void this.plugin.saveSettings(); - }); - }); - - new Setting(containerEl) - .setName(t('settings.testConnection.name')) - .setDesc(t('settings.testConnection.desc', { service: getServiceName(this.plugin.settings) })) - .addButton(button => button - .setButtonText(t('settings.testConnection.button')) - .onClick(async () => { - try { - const result = await this.plugin.testConnection(); - if (!result.repoOk) { - new Notice(t('settings.testConnection.failed', { reason: result.error ?? t('settings.testConnection.failed.unreachable') })); - } else if (!result.branchOk) { - new Notice( - t('settings.testConnection.branchNotFound.notice', { branch: this.plugin.settings.branch }), - 8000 - ); - } else { - new Notice(t('settings.testConnection.success', { service: getServiceName(this.plugin.settings) })); - } - } catch (e: unknown) { - const message = e instanceof Error ? e.message : String(e); - new Notice(t('settings.testConnection.failed', { reason: message })); - } - })); - - this.scheduleConnectionTest(); - } - - // Token fields are masked like a password input (with a toggle to reveal - // them) since they're secrets that shouldn't sit in plaintext on screen - // during screen shares, recordings, or shared machines. - private addTokenSetting(containerEl: HTMLElement, name: string, desc: string, getValue: () => string, onChange: (value: string) => void): void { - let textComponent: TextComponent; - new Setting(containerEl) - .setName(name) - .setDesc(desc) - .addText(text => { - textComponent = text; - text.inputEl.type = 'password'; - text.setPlaceholder(t('settings.token.placeholder')) - .setValue(getValue()) - .onChange(onChange); - }) - .addExtraButton(btn => { - btn.setIcon('eye') - .setTooltip(t('settings.token.show')) - .onClick(() => { - const revealing = textComponent.inputEl.type === 'password'; - textComponent.inputEl.type = revealing ? 'text' : 'password'; - btn.setIcon(revealing ? 'eye-off' : 'eye'); - btn.setTooltip(revealing ? t('settings.token.hide') : t('settings.token.show')); - }); - }); - } - - private displayGitLabSettings(containerEl: HTMLElement): void { - this.addTokenSetting( - containerEl, - t('settings.gitlab.token.name'), - t('settings.gitlab.token.desc'), - () => this.plugin.settings.gitlabToken, - (value) => { - this.plugin.settings.gitlabToken = value; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.scheduleConnectionTest(); - } - ); - - new Setting(containerEl) - .setName(t('settings.gitlab.baseUrl.name')) - .setDesc(t('settings.gitlab.baseUrl.desc')) - .addText(text => text - .setPlaceholder('https://gitlab.com') - .setValue(this.plugin.settings.gitlabBaseUrl) - .onChange((value) => { - this.plugin.settings.gitlabBaseUrl = value || 'https://gitlab.com'; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.scheduleConnectionTest(); - })); - - new Setting(containerEl) - .setName(t('settings.gitlab.projectId.name')) - .setDesc(t('settings.gitlab.projectId.desc')) - .addText(text => text - .setPlaceholder(t('settings.gitlab.projectId.placeholder')) - .setValue(this.plugin.settings.projectId) - .onChange((value) => { - this.plugin.settings.projectId = value; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.scheduleConnectionTest(); - })); - } - - private displayGiteaSettings(containerEl: HTMLElement): void { - this.addTokenSetting( - containerEl, - t('settings.gitea.token.name'), - t('settings.gitea.token.desc'), - () => this.plugin.settings.giteaToken, - (value) => { - this.plugin.settings.giteaToken = value; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.scheduleConnectionTest(); - } - ); - - new Setting(containerEl) - .setName(t('settings.gitea.baseUrl.name')) - .setDesc(t('settings.gitea.baseUrl.desc')) - .addText(text => text - .setPlaceholder('https://gitea.example.com') - .setValue(this.plugin.settings.giteaBaseUrl) - .onChange((value) => { - this.plugin.settings.giteaBaseUrl = value || 'https://gitea.example.com'; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.scheduleConnectionTest(); - })); - - new Setting(containerEl) - .setName(t('settings.repoOwner.name')) - .setDesc(t('settings.repoOwner.desc.gitea')) - .addText(text => text - .setPlaceholder(t('settings.repoOwner.placeholder')) - .setValue(this.plugin.settings.giteaOwner) - .onChange((value) => { - this.plugin.settings.giteaOwner = value; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.scheduleConnectionTest(); - })); - - new Setting(containerEl) - .setName(t('settings.repoName.name')) - .setDesc(t('settings.repoName.desc.gitea')) - .addText(text => text - .setPlaceholder(t('settings.repoName.placeholder')) - .setValue(this.plugin.settings.giteaRepo) - .onChange((value) => { - this.plugin.settings.giteaRepo = value; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.scheduleConnectionTest(); - })); - } - - private displayGitHubSettings(containerEl: HTMLElement): void { - this.addTokenSetting( - containerEl, - t('settings.github.token.name'), - t('settings.github.token.desc'), - () => this.plugin.settings.githubToken, - (value) => { - this.plugin.settings.githubToken = value; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.scheduleConnectionTest(); - } - ); - - new Setting(containerEl) - .setName(t('settings.repoOwner.name')) - .setDesc(t('settings.repoOwner.desc.github')) - .addText(text => text - .setPlaceholder(t('settings.repoOwner.placeholder')) - .setValue(this.plugin.settings.githubOwner) - .onChange((value) => { - this.plugin.settings.githubOwner = value; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.scheduleConnectionTest(); - })); - - new Setting(containerEl) - .setName(t('settings.repoName.name')) - .setDesc(t('settings.repoName.desc.github')) - .addText(text => text - .setPlaceholder(t('settings.repoName.placeholder')) - .setValue(this.plugin.settings.githubRepo) - .onChange((value) => { - this.plugin.settings.githubRepo = value; - void this.plugin.saveSettings(); - this.plugin.initializeGitService(); - this.scheduleConnectionTest(); - })); - } -} diff --git a/src/settings.ts b/src/settings.ts index 995e4cc..f956517 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -1,8 +1,15 @@ -export * from './settings-implementation'; +// Public compatibility surface: re-exports the settings model/helpers and the +// settings UI so existing `from './settings'` / `from '../settings'` imports +// across the codebase keep working unchanged. See src/settings/ (model, +// helpers) and src/ui/settings/GitLabSyncSettingTab.ts for the actual +// implementations; nothing else should be added directly to this file. +export * from './settings/model'; +export * from './settings/helpers'; +export type { SettingsHost } from './ui/settings/GitLabSyncSettingTab'; import { GitLabSyncSettingTab as ImperativeGitLabSyncSettingTab, -} from './settings-implementation'; +} from './ui/settings/GitLabSyncSettingTab'; /** * Keep the existing imperative settings UI on Obsidian's display() lifecycle. diff --git a/src/settings/helpers.ts b/src/settings/helpers.ts new file mode 100644 index 0000000..fded38c --- /dev/null +++ b/src/settings/helpers.ts @@ -0,0 +1,28 @@ +import type { GitLabFilesPushSettings, SymlinkHandling, SyncMetadata } from './model'; + +/** + * Metadata written before `lastKnownPath` was introduced used its record key + * as the path. Keep that format eligible for rename reconciliation. + */ +export function isSyncMetadataAtPath(metadata: SyncMetadata | undefined, path: string): metadata is SyncMetadata { + return metadata !== undefined && (metadata.lastKnownPath === undefined || metadata.lastKnownPath === path); +} + +export function getServiceName(settings: GitLabFilesPushSettings): string { + if (settings.serviceType === 'gitlab') return 'GitLab'; + if (settings.serviceType === 'gitea') return 'Gitea'; + return 'GitHub'; +} + +/** + * Resolves the symlink behavior that actually applies. Only GitHub can create or + * push real symlinks (it has the Git Data API); on other providers "real" is not + * possible, so it is treated as "skip" to avoid silently turning links into + * ordinary files. + */ +export function getEffectiveSymlinkHandling(settings: GitLabFilesPushSettings): SymlinkHandling { + if (settings.symlinkHandling === 'real' && settings.serviceType !== 'github') { + return 'skip'; + } + return settings.symlinkHandling; +} diff --git a/src/settings/model.ts b/src/settings/model.ts new file mode 100644 index 0000000..ca91cfe --- /dev/null +++ b/src/settings/model.ts @@ -0,0 +1,78 @@ +import type { LanguageSetting } from '../i18n'; + +export interface SyncMetadata { + lastSyncedSha: string; + lastSyncedAt: number; + lastKnownPath?: string; + /** + * Set when the vault's 'rename' event moved this entry from another path + * and the move hasn't been pushed yet. Always the path still live on the + * remote — a chain of renames (A→B→C) collapses to this pointing at A, not + * the most recent hop, so pushing deletes the right remote path. + */ + renamedFrom?: string; +} + +export type GitServiceType = 'gitlab' | 'github' | 'gitea'; + +/** + * How symbolic links (Git blobs with mode 120000) are synced: + * - 'real': recreate a real OS symlink on desktop; on mobile (no symlink API) + * fall back to syncing the link target's content as a normal file. + * - 'follow': always sync the target file's content as a normal file. + * - 'skip': ignore symlinks entirely. + */ +export type SymlinkHandling = 'real' | 'follow' | 'skip'; + +export interface GitLabFilesPushSettings { + serviceType: GitServiceType; + gitlabToken: string; + gitlabBaseUrl: string; + projectId: string; + githubToken: string; + githubOwner: string; + githubRepo: string; + giteaToken: string; + giteaBaseUrl: string; + giteaOwner: string; + giteaRepo: string; + branch: string; + syncMetadata: Record; + rootPath: string; + vaultFolder: string; + symlinkHandling: SymlinkHandling; + /** Multi-line, .gitignore-style patterns applied locally, in addition to the remote repo's .gitignore rules. */ + ignorePatterns: string; + /** Plugin version last seen by this vault, used to show a "what's new" tip after an update. */ + lastSeenVersion: string; + /** Version whose "what's new" banner in the settings tab has been dismissed, if any. */ + bannerDismissedVersion: string; + /** UI language. 'system' follows Obsidian's display language, falling back to English if unsupported. */ + language: LanguageSetting; + /** Refresh the sync status automatically after Obsidian finishes loading. */ + autoRefreshOnStartup: boolean; +} + +export const DEFAULT_SETTINGS: GitLabFilesPushSettings = { + serviceType: 'gitlab', + gitlabToken: '', + gitlabBaseUrl: 'https://gitlab.com', + projectId: '', + githubToken: '', + githubOwner: '', + githubRepo: '', + giteaToken: '', + giteaBaseUrl: '', + giteaOwner: '', + giteaRepo: '', + rootPath: '', + branch: 'main', + syncMetadata: {}, + vaultFolder: '', + symlinkHandling: 'real', + ignorePatterns: '', + lastSeenVersion: '', + bannerDismissedVersion: '', + language: 'system', + autoRefreshOnStartup: true, +}; diff --git a/src/ui/settings/GitLabSyncSettingTab.ts b/src/ui/settings/GitLabSyncSettingTab.ts new file mode 100644 index 0000000..04df676 --- /dev/null +++ b/src/ui/settings/GitLabSyncSettingTab.ts @@ -0,0 +1,514 @@ +import { App, Plugin, PluginSettingTab, Setting, Notice, TextComponent, ButtonComponent } from 'obsidian'; +import type { ConnectionStatus } from '../../main'; +// Type-only: RemoteFolderSuggest.attach() still requires the concrete plugin +// class for its own gitService/settings reads. Widening SettingsHost to cover +// that unrelated widget's needs would leak scope into this PR; narrowing +// RemoteFolderSuggest itself is a separate cleanup, not part of this one. +import type GitLabFilesPush from '../../main'; +import type { ConnectionTestResult } from '../../services/git-service-base'; +import { FolderSuggest } from '../FolderSuggest'; +import { RemoteFolderSuggest } from '../RemoteFolderSuggest'; +import { WhatsNewModal } from '../WhatsNewModal'; +import { t, setLanguageOverride, type LanguageSetting } from '../../i18n'; +import { CHANGELOG, entryText } from '../../changelog'; +import type { GitLabFilesPushSettings, GitServiceType, SymlinkHandling } from '../../settings/model'; +import { getServiceName, getEffectiveSymlinkHandling } from '../../settings/helpers'; + +// Minimal shape of Obsidian >= 1.13's SettingDefinitionItem. Declared locally so +// the plugin still type-checks against older Obsidian typings (minAppVersion +// 1.11.0), where this type does not exist. Obsidian only calls +// getSettingDefinitions() on versions that understand it. +interface SettingDefinitionItem { + name: string; + render: (setting: unknown, group: { listEl: HTMLElement }) => void; +} + +/** + * Narrow view of the plugin host this settings tab actually needs, so this UI + * layer depends on a small behavioral contract instead of the concrete + * `GitLabFilesPush` class -- keeps this file free to be tested against a + * plain stub and never creates a `settings UI -> main.ts` value dependency. + */ +export interface SettingsHost { + settings: GitLabFilesPushSettings; + manifest: { version: string }; + saveSettings(): Promise; + initializeGitService(): void; + testConnection(): Promise; + activateSourceControlView(): Promise; + onConnectionStatusChange(listener: (status: ConnectionStatus) => void): () => void; +} + +const CONNECTION_TEST_DEBOUNCE_MS = 800; + +export class GitLabSyncSettingTab extends PluginSettingTab { + private statusBadgeEl: HTMLElement | null = null; + private connectionTestTimer: number | null = null; + private unsubscribeConnectionStatus: (() => void) | null = null; + + /** + * `plugin` and `host` are almost always the same object; kept as separate + * parameters (rather than `Plugin & SettingsHost`) so `this.host`'s type + * only carries SettingsHost's own `settings` declaration -- an + * intersection with `Plugin` would also carry Plugin's version-gated + * `settings?: unknown` (Obsidian 1.13+) and trip this repo's + * `obsidianmd/no-unsupported-api` guard on every `this.host.settings` read. + */ + constructor(app: App, plugin: Plugin, private readonly host: SettingsHost) { + super(app, plugin); + } + + // The status badge mirrors the plugin's shared connection status (also + // driving the status bar item) instead of running its own test, so both + // stay in sync and don't race separate requests against the remote API. + hide(): void { + this.unsubscribeConnectionStatus?.(); + this.unsubscribeConnectionStatus = null; + if (this.connectionTestTimer) { + window.clearTimeout(this.connectionTestTimer); + this.connectionTestTimer = null; + } + } + + // Kept as a fallback for Obsidian < 1.13.0 (older than 1.13, down to + // minAppVersion 1.11.0), which don't know about getSettingDefinitions() + // and always call display(). + display(): void { + this.renderSettings(this.containerEl); + } + + getSettingDefinitions(): SettingDefinitionItem[] { + return [{ + name: '', + render: (_setting, group) => { + this.renderSettings(group.listEl); + } + }]; + } + + private refresh(): void { + // update() only exists on Obsidian >= 1.13. On older versions (down to + // minAppVersion 1.11.0) re-render manually instead. Accessed via a cast + // so this compiles against the 1.11 typings, which lack update(). + const maybeUpdate = (this as { update?: () => void }).update; + if (typeof maybeUpdate === 'function') { + maybeUpdate.call(this); + } else { + this.renderSettings(this.containerEl); + } + } + + // Persistent (until dismissed) banner surfacing the current version's notable + // highlights right at the top of the settings tab. Dismissing this only hides + // the attention banner; release history remains available from Settings. + private renderWhatsNewBanner(containerEl: HTMLElement): void { + const currentVersion = this.host.manifest.version; + if (this.host.settings.bannerDismissedVersion === currentVersion) return; + + const release = CHANGELOG.find(r => r.version === currentVersion); + const notableEntries = release?.entries.filter(entry => entry.notable) ?? []; + if (notableEntries.length === 0) return; + + // Onboarding releases already teach their mental model in the modal's + // step-by-step layout — keep the banner itself to a couple of highlights + // rather than repeating every notable entry. + const bannerEntries = release?.onboarding ? notableEntries.slice(0, 2) : notableEntries; + + const banner = containerEl.createDiv({ cls: 'gfs-whats-new-banner' }); + const textEl = banner.createDiv({ cls: 'gfs-whats-new-banner-text' }); + textEl.createEl('strong', { text: t('settings.whatsNewBanner.title', { version: currentVersion }) }); + const list = textEl.createEl('ul', { cls: 'gfs-whats-new-banner-list' }); + for (const entry of bannerEntries) { + list.createEl('li', { text: entryText(entry) }); + } + const viewBtn = new ButtonComponent(textEl) + .setButtonText(t('settings.whatsNewBanner.view')) + .onClick(() => { + new WhatsNewModal(this.app, CHANGELOG, () => void this.host.activateSourceControlView()).open(); + }); + viewBtn.buttonEl.addClass('gfs-whats-new-banner-view'); + + const dismissBtn = banner.createEl('button', { + cls: 'gfs-whats-new-banner-dismiss', + text: '×', + attr: { 'aria-label': t('settings.whatsNewBanner.dismiss') } + }); + dismissBtn.addEventListener('click', () => { + void (async () => { + this.host.settings.bannerDismissedVersion = currentVersion; + await this.host.saveSettings(); + this.refresh(); + })(); + }); + } + + private renderReleaseHistorySetting(containerEl: HTMLElement): void { + new Setting(containerEl) + .setName(t('settings.releaseHistory.name')) + .setDesc(t('settings.releaseHistory.desc')) + .addButton(button => button + .setButtonText(t('settings.releaseHistory.button')) + .onClick(() => { + new WhatsNewModal(this.app, CHANGELOG, () => void this.host.activateSourceControlView()).open(); + })); + } + + // Rebuilding the whole settings tab (renderSettings) to refresh the badge + // would empty and recreate every field, stealing focus mid-typing. The + // badge element is instead created once per renderSettings pass and + // updated in place by setStatusBadge(), driven by the plugin's shared + // connection status (see main.ts) so it stays in sync with the status bar. + private renderConnectionStatus(containerEl: HTMLElement): void { + this.statusBadgeEl = containerEl.createDiv({ cls: 'gfs-connection-status' }); + this.unsubscribeConnectionStatus?.(); + this.unsubscribeConnectionStatus = this.host.onConnectionStatusChange((status) => this.setStatusBadge(status)); + } + + private setStatusBadge(status: ConnectionStatus): void { + const badge = this.statusBadgeEl; + if (!badge) return; + + badge.removeClass('is-checking', 'is-connected', 'is-disconnected'); + badge.addClass(`is-${status.state}`); + + const labels: Record = { + checking: t('settings.connectionStatus.checking'), + connected: t('settings.connectionStatus.connected'), + disconnected: t('settings.connectionStatus.disconnected') + }; + const label = labels[status.state]; + badge.setText(status.detail ? t('settings.connectionStatus.withDetail', { label, detail: status.detail }) : label); + } + + // Debounced so token/branch fields (which call this on every keystroke) + // don't hit the remote API on every character typed. + private scheduleConnectionTest(): void { + if (this.connectionTestTimer) { + window.clearTimeout(this.connectionTestTimer); + } + this.connectionTestTimer = window.setTimeout(() => { + this.connectionTestTimer = null; + void this.host.testConnection(); + }, CONNECTION_TEST_DEBOUNCE_MS); + } + + private renderSettings(containerEl: HTMLElement): void { + containerEl.empty(); + + this.renderWhatsNewBanner(containerEl); + this.renderReleaseHistorySetting(containerEl); + this.renderConnectionStatus(containerEl); + + new Setting(containerEl) + .setName(t('settings.language.name')) + .setDesc(t('settings.language.desc')) + .addDropdown(dropdown => dropdown + .addOption('system', t('settings.language.option.system')) + .addOption('en', t('settings.language.option.en')) + .addOption('zh-tw', t('settings.language.option.zhTw')) + .addOption('zh-cn', t('settings.language.option.zhCn')) + .setValue(this.host.settings.language) + .onChange((value: string) => { + this.host.settings.language = value as LanguageSetting; + void this.host.saveSettings(); + setLanguageOverride(this.host.settings.language); + this.refresh(); + })); + + new Setting(containerEl) + .setName(t('settings.gitService.name')) + .setDesc(t('settings.gitService.desc')) + .addDropdown(dropdown => dropdown + .addOption('gitlab', 'GitLab') + .addOption('github', 'GitHub') + .addOption('gitea', 'Gitea') + .setValue(this.host.settings.serviceType) + .onChange((value: string) => { + this.host.settings.serviceType = value as GitServiceType; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.refresh(); + })); + + new Setting(containerEl).setName('').setHeading(); + + if (this.host.settings.serviceType === 'gitlab') { + this.displayGitLabSettings(containerEl); + } else if (this.host.settings.serviceType === 'gitea') { + this.displayGiteaSettings(containerEl); + } else { + this.displayGitHubSettings(containerEl); + } + + new Setting(containerEl) + .setName(t('settings.branch.name')) + .setDesc(t('settings.branch.desc')) + .addText(text => text + .setPlaceholder(t('settings.branch.placeholder')) + .setValue(this.host.settings.branch) + .onChange((value) => { + this.host.settings.branch = value || 'main'; + void this.host.saveSettings(); + this.scheduleConnectionTest(); + })); + + new Setting(containerEl) + .setName(t('settings.rootPath.name')) + .setDesc(t('settings.rootPath.desc')) + .addText(text => { + text.setPlaceholder(t('settings.rootPath.placeholder')) + .setValue(this.host.settings.rootPath) + .onChange((value) => { + this.host.settings.rootPath = value.replace(/^\/|\/$/g, ''); + void this.host.saveSettings(); + this.host.initializeGitService(); + }); + RemoteFolderSuggest.attach(this.app, text.inputEl, this.host as unknown as GitLabFilesPush); + }); + + new Setting(containerEl) + .setName(t('settings.vaultFolder.name')) + .setDesc(t('settings.vaultFolder.desc')) + .addText(text => { + text.setPlaceholder(t('settings.vaultFolder.placeholder')) + .setValue(this.host.settings.vaultFolder) + .onChange((value) => { + this.host.settings.vaultFolder = value.replace(/^\/|\/$/g, ''); + void this.host.saveSettings(); + }); + FolderSuggest.attach(this.app, text.inputEl); + }); + + new Setting(containerEl) + .setName(t('settings.autoRefreshOnStartup.name')) + .setDesc(t('settings.autoRefreshOnStartup.desc')) + .addToggle(toggle => toggle + .setValue(this.host.settings.autoRefreshOnStartup) + .onChange((value) => { + this.host.settings.autoRefreshOnStartup = value; + void this.host.saveSettings(); + })); + + new Setting(containerEl) + .setName(t('settings.ignorePatterns.name')) + .setDesc(t('settings.ignorePatterns.desc')) + .addTextArea(text => { + text.setPlaceholder(`${this.app.vault.configDir}/\n*.tmp`) + .setValue(this.host.settings.ignorePatterns) + .onChange((value) => { + this.host.settings.ignorePatterns = value; + void this.host.saveSettings(); + }); + text.inputEl.rows = 4; + }); + + // "Real symlink" needs the Git Data API, which only GitHub offers. For + // other providers, offer follow/skip only so the option can't mislead. + const supportsRealSymlink = this.host.settings.serviceType === 'github'; + new Setting(containerEl) + .setName(t('settings.symlinks.name')) + .setDesc(supportsRealSymlink + ? t('settings.symlinks.desc.supported') + : t('settings.symlinks.desc.unsupported')) + .addDropdown(dropdown => { + if (supportsRealSymlink) dropdown.addOption('real', t('settings.symlinks.option.real')); + dropdown + .addOption('follow', t('settings.symlinks.option.follow')) + .addOption('skip', t('settings.symlinks.option.skip')) + .setValue(getEffectiveSymlinkHandling(this.host.settings)) + .onChange((value: string) => { + this.host.settings.symlinkHandling = value as SymlinkHandling; + void this.host.saveSettings(); + }); + }); + + new Setting(containerEl) + .setName(t('settings.testConnection.name')) + .setDesc(t('settings.testConnection.desc', { service: getServiceName(this.host.settings) })) + .addButton(button => button + .setButtonText(t('settings.testConnection.button')) + .onClick(async () => { + try { + const result = await this.host.testConnection(); + if (!result.repoOk) { + new Notice(t('settings.testConnection.failed', { reason: result.error ?? t('settings.testConnection.failed.unreachable') })); + } else if (!result.branchOk) { + new Notice( + t('settings.testConnection.branchNotFound.notice', { branch: this.host.settings.branch }), + 8000 + ); + } else { + new Notice(t('settings.testConnection.success', { service: getServiceName(this.host.settings) })); + } + } catch (e: unknown) { + const message = e instanceof Error ? e.message : String(e); + new Notice(t('settings.testConnection.failed', { reason: message })); + } + })); + + this.scheduleConnectionTest(); + } + + // Token fields are masked like a password input (with a toggle to reveal + // them) since they're secrets that shouldn't sit in plaintext on screen + // during screen shares, recordings, or shared machines. + private addTokenSetting(containerEl: HTMLElement, name: string, desc: string, getValue: () => string, onChange: (value: string) => void): void { + let textComponent: TextComponent; + new Setting(containerEl) + .setName(name) + .setDesc(desc) + .addText(text => { + textComponent = text; + text.inputEl.type = 'password'; + text.setPlaceholder(t('settings.token.placeholder')) + .setValue(getValue()) + .onChange(onChange); + }) + .addExtraButton(btn => { + btn.setIcon('eye') + .setTooltip(t('settings.token.show')) + .onClick(() => { + const revealing = textComponent.inputEl.type === 'password'; + textComponent.inputEl.type = revealing ? 'text' : 'password'; + btn.setIcon(revealing ? 'eye-off' : 'eye'); + btn.setTooltip(revealing ? t('settings.token.hide') : t('settings.token.show')); + }); + }); + } + + private displayGitLabSettings(containerEl: HTMLElement): void { + this.addTokenSetting( + containerEl, + t('settings.gitlab.token.name'), + t('settings.gitlab.token.desc'), + () => this.host.settings.gitlabToken, + (value) => { + this.host.settings.gitlabToken = value; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.scheduleConnectionTest(); + } + ); + + new Setting(containerEl) + .setName(t('settings.gitlab.baseUrl.name')) + .setDesc(t('settings.gitlab.baseUrl.desc')) + .addText(text => text + .setPlaceholder('https://gitlab.com') + .setValue(this.host.settings.gitlabBaseUrl) + .onChange((value) => { + this.host.settings.gitlabBaseUrl = value || 'https://gitlab.com'; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.scheduleConnectionTest(); + })); + + new Setting(containerEl) + .setName(t('settings.gitlab.projectId.name')) + .setDesc(t('settings.gitlab.projectId.desc')) + .addText(text => text + .setPlaceholder(t('settings.gitlab.projectId.placeholder')) + .setValue(this.host.settings.projectId) + .onChange((value) => { + this.host.settings.projectId = value; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.scheduleConnectionTest(); + })); + } + + private displayGiteaSettings(containerEl: HTMLElement): void { + this.addTokenSetting( + containerEl, + t('settings.gitea.token.name'), + t('settings.gitea.token.desc'), + () => this.host.settings.giteaToken, + (value) => { + this.host.settings.giteaToken = value; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.scheduleConnectionTest(); + } + ); + + new Setting(containerEl) + .setName(t('settings.gitea.baseUrl.name')) + .setDesc(t('settings.gitea.baseUrl.desc')) + .addText(text => text + .setPlaceholder('https://gitea.example.com') + .setValue(this.host.settings.giteaBaseUrl) + .onChange((value) => { + this.host.settings.giteaBaseUrl = value || 'https://gitea.example.com'; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.scheduleConnectionTest(); + })); + + new Setting(containerEl) + .setName(t('settings.repoOwner.name')) + .setDesc(t('settings.repoOwner.desc.gitea')) + .addText(text => text + .setPlaceholder(t('settings.repoOwner.placeholder')) + .setValue(this.host.settings.giteaOwner) + .onChange((value) => { + this.host.settings.giteaOwner = value; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.scheduleConnectionTest(); + })); + + new Setting(containerEl) + .setName(t('settings.repoName.name')) + .setDesc(t('settings.repoName.desc.gitea')) + .addText(text => text + .setPlaceholder(t('settings.repoName.placeholder')) + .setValue(this.host.settings.giteaRepo) + .onChange((value) => { + this.host.settings.giteaRepo = value; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.scheduleConnectionTest(); + })); + } + + private displayGitHubSettings(containerEl: HTMLElement): void { + this.addTokenSetting( + containerEl, + t('settings.github.token.name'), + t('settings.github.token.desc'), + () => this.host.settings.githubToken, + (value) => { + this.host.settings.githubToken = value; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.scheduleConnectionTest(); + } + ); + + new Setting(containerEl) + .setName(t('settings.repoOwner.name')) + .setDesc(t('settings.repoOwner.desc.github')) + .addText(text => text + .setPlaceholder(t('settings.repoOwner.placeholder')) + .setValue(this.host.settings.githubOwner) + .onChange((value) => { + this.host.settings.githubOwner = value; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.scheduleConnectionTest(); + })); + + new Setting(containerEl) + .setName(t('settings.repoName.name')) + .setDesc(t('settings.repoName.desc.github')) + .addText(text => text + .setPlaceholder(t('settings.repoName.placeholder')) + .setValue(this.host.settings.githubRepo) + .onChange((value) => { + this.host.settings.githubRepo = value; + void this.host.saveSettings(); + this.host.initializeGitService(); + this.scheduleConnectionTest(); + })); + } +} diff --git a/tests/settings.test.ts b/tests/settings.test.ts new file mode 100644 index 0000000..3fcce73 --- /dev/null +++ b/tests/settings.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from 'vitest'; +import * as settingsCompat from '../src/settings'; +import * as settingsModel from '../src/settings/model'; +import * as settingsHelpers from '../src/settings/helpers'; + +describe('settings module split', () => { + it('re-exports the model and helpers from src/settings.ts unchanged', () => { + expect(settingsCompat.DEFAULT_SETTINGS).toBe(settingsModel.DEFAULT_SETTINGS); + expect(settingsCompat.getServiceName).toBe(settingsHelpers.getServiceName); + expect(settingsCompat.getEffectiveSymlinkHandling).toBe(settingsHelpers.getEffectiveSymlinkHandling); + expect(settingsCompat.isSyncMetadataAtPath).toBe(settingsHelpers.isSyncMetadataAtPath); + }); + + it('keeps DEFAULT_SETTINGS shape/values unchanged by the split', () => { + expect(settingsModel.DEFAULT_SETTINGS).toEqual({ + serviceType: 'gitlab', + gitlabToken: '', + gitlabBaseUrl: 'https://gitlab.com', + projectId: '', + githubToken: '', + githubOwner: '', + githubRepo: '', + giteaToken: '', + giteaBaseUrl: '', + giteaOwner: '', + giteaRepo: '', + rootPath: '', + branch: 'main', + syncMetadata: {}, + vaultFolder: '', + symlinkHandling: 'real', + ignorePatterns: '', + lastSeenVersion: '', + bannerDismissedVersion: '', + language: 'system', + autoRefreshOnStartup: true, + }); + }); + + it('getServiceName still maps every GitServiceType to its display name', () => { + expect(settingsHelpers.getServiceName({ ...settingsModel.DEFAULT_SETTINGS, serviceType: 'gitlab' })).toBe('GitLab'); + expect(settingsHelpers.getServiceName({ ...settingsModel.DEFAULT_SETTINGS, serviceType: 'github' })).toBe('GitHub'); + expect(settingsHelpers.getServiceName({ ...settingsModel.DEFAULT_SETTINGS, serviceType: 'gitea' })).toBe('Gitea'); + }); + + it('getEffectiveSymlinkHandling still downgrades "real" to "skip" on non-GitHub providers', () => { + const base = { ...settingsModel.DEFAULT_SETTINGS, symlinkHandling: 'real' as const }; + expect(settingsHelpers.getEffectiveSymlinkHandling({ ...base, serviceType: 'github' })).toBe('real'); + expect(settingsHelpers.getEffectiveSymlinkHandling({ ...base, serviceType: 'gitlab' })).toBe('skip'); + expect(settingsHelpers.getEffectiveSymlinkHandling({ ...base, serviceType: 'gitea' })).toBe('skip'); + }); + + it('isSyncMetadataAtPath still accepts legacy (keyed-by-path, no lastKnownPath) metadata', () => { + expect(settingsHelpers.isSyncMetadataAtPath({ lastSyncedSha: 'sha', lastSyncedAt: 0 }, 'a.md')).toBe(true); + expect(settingsHelpers.isSyncMetadataAtPath({ lastSyncedSha: 'sha', lastSyncedAt: 0, lastKnownPath: 'b.md' }, 'a.md')).toBe(false); + expect(settingsHelpers.isSyncMetadataAtPath(undefined, 'a.md')).toBe(false); + }); +}); diff --git a/tests/ui/SettingsConnectionStatus.test.ts b/tests/ui/SettingsConnectionStatus.test.ts index 1e40b51..01ea074 100644 --- a/tests/ui/SettingsConnectionStatus.test.ts +++ b/tests/ui/SettingsConnectionStatus.test.ts @@ -58,7 +58,8 @@ describe('GitLabSyncSettingTab connection status badge', () => { it('shows checking then connected after opening the tab', async () => { const testConnection = vi.fn().mockResolvedValue({ repoOk: true, branchOk: true }); - const tab = new GitLabSyncSettingTab(new App(), createPluginStub(testConnection)); + const plugin = createPluginStub(testConnection); + const tab = new GitLabSyncSettingTab(new App(), plugin, plugin); tab.containerEl = createContainer(); tab.display(); @@ -78,7 +79,7 @@ describe('GitLabSyncSettingTab connection status badge', () => { it('debounces repeated field edits into a single connection test', async () => { const testConnection = vi.fn().mockResolvedValue({ repoOk: false, branchOk: false, error: 'bad token' }); const plugin = createPluginStub(testConnection); - const tab = new GitLabSyncSettingTab(new App(), plugin); + const tab = new GitLabSyncSettingTab(new App(), plugin, plugin); tab.containerEl = createContainer(); tab.display(); @@ -108,7 +109,7 @@ describe('GitLabSyncSettingTab ignore patterns setting', () => { it('renders a textarea seeded with the saved ignorePatterns value', async () => { const plugin = createPluginStub(vi.fn().mockResolvedValue({ repoOk: true, branchOk: true })); plugin.settings.ignorePatterns = 'draft/\n*.tmp'; - const tab = new GitLabSyncSettingTab(new App(), plugin); + const tab = new GitLabSyncSettingTab(new App(), plugin, plugin); tab.containerEl = createContainer(); vi.useFakeTimers(); @@ -127,7 +128,7 @@ describe('GitLabSyncSettingTab release history', () => { const plugin = createPluginStub(vi.fn().mockResolvedValue({ repoOk: true, branchOk: true })); plugin.manifest = { version: '1.5.0' } as GitLabFilesPush['manifest']; plugin.settings.bannerDismissedVersion = '1.5.0'; - const tab = new GitLabSyncSettingTab(new App(), plugin); + const tab = new GitLabSyncSettingTab(new App(), plugin, plugin); tab.containerEl = createContainer(); try { @@ -149,7 +150,7 @@ describe('GitLabSyncSettingTab what\'s new banner', () => { const plugin = createPluginStub(vi.fn().mockResolvedValue({ repoOk: true, branchOk: true })); plugin.manifest = { version } as GitLabFilesPush['manifest']; plugin.settings.bannerDismissedVersion = bannerDismissedVersion; - const tab = new GitLabSyncSettingTab(new App(), plugin); + const tab = new GitLabSyncSettingTab(new App(), plugin, plugin); tab.containerEl = createContainer(); tab.display(); return tab; diff --git a/tests/ui/SettingsObsidian113Compatibility.test.ts b/tests/ui/SettingsObsidian113Compatibility.test.ts index ef3a850..0cc24b7 100644 --- a/tests/ui/SettingsObsidian113Compatibility.test.ts +++ b/tests/ui/SettingsObsidian113Compatibility.test.ts @@ -40,14 +40,16 @@ function renderAsObsidian113(tab: GitLabSyncSettingTab): void { describe('GitLabSyncSettingTab on Obsidian 1.13+', () => { it('returns no declarative definitions until the tab is fully migrated', () => { - const tab = new GitLabSyncSettingTab(new App(), createPluginStub()); + const plugin = createPluginStub(); + const tab = new GitLabSyncSettingTab(new App(), plugin, plugin); expect(tab.getSettingDefinitions()).toEqual([]); }); it('falls back to display() and renders settings instead of a blank page', () => { vi.useFakeTimers(); - const tab = new GitLabSyncSettingTab(new App(), createPluginStub()); + const plugin = createPluginStub(); + const tab = new GitLabSyncSettingTab(new App(), plugin, plugin); tab.containerEl = createContainer(); const displaySpy = vi.spyOn(tab, 'display'); From 0f932049d5a7a518e6587c99218580bd95f0fe11 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 06:33:34 +0000 Subject: [PATCH 04/10] docs: record PR2 item 2 (settings boundary cleanup) session progress Co-Authored-By: Claude Sonnet 5 --- progress.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/progress.md b/progress.md index d623494..d8dd8b4 100644 --- a/progress.md +++ b/progress.md @@ -5,17 +5,17 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont ## Current State **Last Updated:** 2026-09-01 -**Active Feature:** PR2 responsibility cleanup, item 1/5 — Source Control state boundary (no tracked issue number; an ad-hoc follow-up plan on top of `origin/1.6.1`, not in `feature_list.json`). +**Active Feature:** PR2 responsibility cleanup, item 2/5 done — Settings boundary cleanup (no tracked issue number; an ad-hoc follow-up plan on top of `origin/1.6.1`, not in `feature_list.json`). **Branch / PR:** `claude/pr2-source-control-boundary`, branched from `origin/1.6.1` (commit `69e5540`). Not yet pushed or opened as a PR. -**Scope (item 1 only, per the PR2 plan):** `SourceControlViewModel` is now a pure read-only projection — removed its `selection` getter and its constructor's `ChangeRepository.subscribe(... reconcile ...)` wiring. That reconciliation wiring now lives in `createSyncRuntime`, which also drops the redundant explicit `syncSelectionStore.refresh()` call it used to make alongside it (reconcile already supersedes it). Selection mutation (`selectForSync`/`deselectFromSync`/`selectMany`/`deselectMany`/`setSyncAction`/`clearSyncAction`) moved onto `SourceControlActionService`, which now also takes `SyncSelectionStore` in its constructor; `SourceControlView` calls injected callbacks instead of reaching into `SyncSelectionStore` via the ViewModel. Updated the 3 e2e-support call sites that constructed `SourceControlActionService` directly. Deliberately did not touch items 2-5 of the PR2 plan (Settings boundary, item-projection centralization, pull-orchestration reuse, provider contract cleanup) or any UX. +**Scope (item 2, per the PR2 plan):** Split `src/settings-implementation.ts` into `src/settings/model.ts` (types + `DEFAULT_SETTINGS`), `src/settings/helpers.ts` (pure functions), and `src/ui/settings/GitLabSyncSettingTab.ts` (all Obsidian rendering); `src/settings.ts` is now a thin re-export shim so every existing `from './settings'` import is unchanged. `GitLabSyncSettingTab` no longer imports the concrete `GitLabFilesPush` class for its own behavior — it depends on a narrow `SettingsHost` interface instead, with `plugin: Plugin` and `host: SettingsHost` kept as separate constructor parameters (an intersection type would re-trip `obsidianmd/no-unsupported-api` on `Plugin`'s own version-gated `settings` field). One remaining wart — `RemoteFolderSuggest.attach` still needs the concrete plugin class — is called out with a type-only cast + comment rather than fixed here (out of scope). No settings UX change. Did **not** touch `eslint.config.mts` — added an architecture-guard rule for this boundary, then reverted it per user feedback (config changes need to be proposed, not made inline). -**Next:** items 2-5 of the PR2 plan, one at a time, each its own commit — Settings boundary cleanup (item 2) is next up. +**Next:** items 3-5 of the PR2 plan, one at a time, each its own commit — item 3 (centralize Source Control item projection via `SourceControlViewModel.getItem()`) is next up. Below that: the previous "Outstanding Items"/"Verification Evidence" entries track separate, still-open work on PR #129 / `claude/source-control-foundation`, Issue #143, and `claude/fix-source-control-explicit-sync-intent` — not superseded by this entry, carried over from the base branch history. - `npx eslint .` — 0 errors. -- `npx vitest run` — 74 files / 940 tests passed (up from 933; added SourceControlActionService selection-mutation tests and createSyncRuntime reconciliation-wiring tests). +- `npx vitest run` — 75 files / 945 tests passed (up from 940; added `tests/settings.test.ts` for the model/helpers split). - `npm run build` (tsc + Obsidian 1.11.0 compat typecheck + esbuild) — passed. ## Outstanding Items From 2527f267959ea454b067c3dd2e529e9d01882b2b Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 06:42:59 +0000 Subject: [PATCH 05/10] refactor(source-control): centralize item projection Adds SourceControlViewModel.getItem(id), the single SourceControlItem projection path for callers that need one row's current state by id. SourceControlItemView.refreshOpenDiffTab() no longer hand-rolls a SourceControlItem with hardcoded isSelectedForSync: false / operationStatus: 'idle' / a fresh resolveSyncAction() call -- it now reads the ViewModel's real projection, so a queued/running row's open diff tab refresh reflects its actual state instead of silently reporting defaults. SourceControlView's mobile detail loadAndRenderDiff() similarly drops its two-getState() lookup (needed to also catch 'synced' rows) in favor of the unfiltered getItem(). Co-Authored-By: Claude Sonnet 5 --- .../source-control/SourceControlViewModel.ts | 13 ++++++++ .../source-control/SourceControlItemView.ts | 19 ++++------- src/ui/source-control/SourceControlView.ts | 3 +- .../SourceControlViewModel.test.ts | 33 +++++++++++++++++++ .../SourceControlItemView.test.ts | 28 +++++++++++++++- 5 files changed, 80 insertions(+), 16 deletions(-) diff --git a/src/logic/source-control/SourceControlViewModel.ts b/src/logic/source-control/SourceControlViewModel.ts index 96c1baa..62884c4 100644 --- a/src/logic/source-control/SourceControlViewModel.ts +++ b/src/logic/source-control/SourceControlViewModel.ts @@ -68,6 +68,19 @@ export class SourceControlViewModel { }; } + /** + * Projects a single change by id, independent of any filter -- the sole + * projection path for callers (e.g. a diff pane host) that need one + * row's current selection/operation/syncAction state without hand-rolling + * a SourceControlItem themselves. Returns undefined once the change is no + * longer in the repository (e.g. it synced and dropped out, or was + * deleted). + */ + getItem(id: ChangeId): SourceControlItem | undefined { + const change = this.changes.getById(id); + return change ? this.toItem(change) : undefined; + } + /** * Triggers a view-wide refresh through the injected source and records * only its presentation lifecycle. Repository population still happens diff --git a/src/ui/source-control/SourceControlItemView.ts b/src/ui/source-control/SourceControlItemView.ts index 631d037..b750f7e 100644 --- a/src/ui/source-control/SourceControlItemView.ts +++ b/src/ui/source-control/SourceControlItemView.ts @@ -2,7 +2,6 @@ import { ItemView, Platform, TFile, WorkspaceLeaf, debounce } from 'obsidian'; import GitLabFilesPush from '../../main'; import { t } from '../../i18n'; import type { SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; -import { resolveSyncAction } from '../../logic/source-control/ChangeActionPolicy'; import type { FileStatus } from '../../logic/sync-status-service'; import { toChangeId, type ChangeId } from '../../logic/source-control/types'; import { SourceControlView, type SourceControlViewCallbacks } from './SourceControlView'; @@ -122,21 +121,15 @@ export class SourceControlItemView extends ItemView { if (!openPath || openPath !== path) return; const requestId = ++this.diffTabRequestSeq; void (async () => { - // Project the repository row into the full item shape the diff - // loader consumes; the repo row dropped means the change is gone - // and the pane clears rather than showing contradictory sides. - const change = this.plugin.changeRepository.getById(toChangeId(path)); - if (!change) { + // ViewModel.getItem() is the single SourceControlItem projection + // owner; undefined means the change dropped out of the + // repository, and the pane clears rather than showing + // contradictory sides. + const item = this.plugin.sourceControlViewModel.getItem(toChangeId(path)); + if (!item) { await this.plugin.openDiffTab(path, null); return; } - const item: SourceControlItem = { - ...change, - isSelectedForSync: false, - operationStatus: 'idle', - syncAction: resolveSyncAction(change.kind), - hasActionOverride: false, - }; const content = await this.plugin.sourceControlActions.loadDiffContent(item); if (requestId !== this.diffTabRequestSeq) return; await this.plugin.openDiffTab(path, content); diff --git a/src/ui/source-control/SourceControlView.ts b/src/ui/source-control/SourceControlView.ts index 96f7821..684a832 100644 --- a/src/ui/source-control/SourceControlView.ts +++ b/src/ui/source-control/SourceControlView.ts @@ -559,8 +559,7 @@ export class SourceControlView { private async loadAndRenderDiff(viewer: DiffViewerHandle, changeId: ChangeId): Promise { if (!this.callbacks.loadDiffContent) return; - const item = this.viewModel.getState('all').items.find(i => i.id === changeId) - ?? this.viewModel.getState('synced', true).items.find(i => i.id === changeId); + const item = this.viewModel.getItem(changeId); if (!item) return; const content = await this.callbacks.loadDiffContent(item); diff --git a/tests/logic/source-control/SourceControlViewModel.test.ts b/tests/logic/source-control/SourceControlViewModel.test.ts index aa70ca2..f41f162 100644 --- a/tests/logic/source-control/SourceControlViewModel.test.ts +++ b/tests/logic/source-control/SourceControlViewModel.test.ts @@ -233,4 +233,37 @@ describe('SourceControlViewModel', () => { expect(selection.getActionOverride(toChangeId('c-1'))).toBeUndefined(); }); }); + + describe('getItem', () => { + it('projects a single change by id, independent of any filter', () => { + const synced: SyncChange = { id: toChangeId('c-1'), path: 'a.md', kind: 'synced' }; + const { viewModel, operations } = buildViewModel([synced]); + operations.start(toChangeId('c-1')); + + // 'synced' kind is excluded from getState('all')/('changes'), but + // getItem() is not a filtered view -- it's the single projection + // path any caller can use to resolve one row directly by id. + const item = viewModel.getItem(toChangeId('c-1')); + expect(item?.id).toBe(toChangeId('c-1')); + expect(item?.kind).toBe('synced'); + expect(item?.operationStatus).toBe('running'); + }); + + it('returns undefined once the change is no longer in the repository', () => { + const { viewModel } = buildViewModel([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }]); + + expect(viewModel.getItem(toChangeId('gone'))).toBeUndefined(); + }); + + it('reflects selection and syncAction override state, same as getState()', () => { + const { viewModel, selection } = buildViewModel([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }]); + selection.selectForSync(toChangeId('c-1')); + selection.setActionOverride(toChangeId('c-1'), 'pull'); + + const item = viewModel.getItem(toChangeId('c-1')); + expect(item?.isSelectedForSync).toBe(true); + expect(item?.syncAction).toBe('pull'); + expect(item?.hasActionOverride).toBe(true); + }); + }); }); diff --git a/tests/ui/source-control/SourceControlItemView.test.ts b/tests/ui/source-control/SourceControlItemView.test.ts index e0941e1..5f1584e 100644 --- a/tests/ui/source-control/SourceControlItemView.test.ts +++ b/tests/ui/source-control/SourceControlItemView.test.ts @@ -45,7 +45,7 @@ function buildPlugin(kind: SyncChangeKind = 'local-only') { diffTabPath, } as unknown as GitLabFilesPush; - return { plugin, repository, selection, sync, push, pull, deleteRemote, deleteLocal, loadDiffContent, openDiffTab, getRemoteFileUrl, status, diffTabPath }; + return { plugin, repository, selection, operations, sync, push, pull, deleteRemote, deleteLocal, loadDiffContent, openDiffTab, getRemoteFileUrl, status, diffTabPath }; } function buildLeaf() { @@ -364,6 +364,32 @@ describe('SourceControlItemView', () => { expect(openDiffTab).toHaveBeenCalledWith('a.md', null); }); + it('refreshes the open diff tab using the ViewModel\'s real selection/operation projection, not hardcoded defaults', async () => { + const { plugin, selection, operations, loadDiffContent, diffTabPath } = buildPlugin('local-modified'); + (diffTabPath as ReturnType).mockReturnValue('a.md'); + // Prior to the getItem() refactor this path hardcoded isSelectedForSync: + // false / operationStatus: 'idle' regardless of actual state. + selection.selectForSync(toChangeId('a.md')); + operations.start(toChangeId('a.md')); + const view = new SourceControlItemView({} as WorkspaceLeaf, plugin); + await view.onOpen(); + + const status = (plugin as unknown as { sync: { status: SyncStatusService } }).sync.status; + status.set({ path: 'a.md', status: 'modified', localContent: 'local', remoteContent: 'remote' }); + await new Promise(resolve => window.setTimeout(resolve, 200)); + loadDiffContent.mockClear(); + + status.set({ path: 'a.md', status: 'synced', localContent: 'remote', remoteContent: 'remote', remoteSha: 'new-sha' }); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + + expect(loadDiffContent).toHaveBeenCalledWith(expect.objectContaining({ + isSelectedForSync: true, + operationStatus: 'running', + })); + }); + describe('row menu delete-remote confirmation', () => { afterEach(() => { document.querySelectorAll('.menu').forEach(el => el.remove()); From c80ea94d48aa94903b7b4dbef2770ddf996e41a3 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 06:43:50 +0000 Subject: [PATCH 06/10] docs: record PR2 item 3 (centralize item projection) session progress Co-Authored-By: Claude Sonnet 5 --- progress.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/progress.md b/progress.md index d8dd8b4..0a3dcd0 100644 --- a/progress.md +++ b/progress.md @@ -5,17 +5,17 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont ## Current State **Last Updated:** 2026-09-01 -**Active Feature:** PR2 responsibility cleanup, item 2/5 done — Settings boundary cleanup (no tracked issue number; an ad-hoc follow-up plan on top of `origin/1.6.1`, not in `feature_list.json`). +**Active Feature:** PR2 responsibility cleanup, item 3/5 done — centralize Source Control item projection (no tracked issue number; an ad-hoc follow-up plan on top of `origin/1.6.1`, not in `feature_list.json`). **Branch / PR:** `claude/pr2-source-control-boundary`, branched from `origin/1.6.1` (commit `69e5540`). Not yet pushed or opened as a PR. -**Scope (item 2, per the PR2 plan):** Split `src/settings-implementation.ts` into `src/settings/model.ts` (types + `DEFAULT_SETTINGS`), `src/settings/helpers.ts` (pure functions), and `src/ui/settings/GitLabSyncSettingTab.ts` (all Obsidian rendering); `src/settings.ts` is now a thin re-export shim so every existing `from './settings'` import is unchanged. `GitLabSyncSettingTab` no longer imports the concrete `GitLabFilesPush` class for its own behavior — it depends on a narrow `SettingsHost` interface instead, with `plugin: Plugin` and `host: SettingsHost` kept as separate constructor parameters (an intersection type would re-trip `obsidianmd/no-unsupported-api` on `Plugin`'s own version-gated `settings` field). One remaining wart — `RemoteFolderSuggest.attach` still needs the concrete plugin class — is called out with a type-only cast + comment rather than fixed here (out of scope). No settings UX change. Did **not** touch `eslint.config.mts` — added an architecture-guard rule for this boundary, then reverted it per user feedback (config changes need to be proposed, not made inline). +**Scope (item 3, per the PR2 plan):** Added `SourceControlViewModel.getItem(id)` as the single `SourceControlItem` projection path by id. `SourceControlItemView.refreshOpenDiffTab()` no longer hand-rolls a `SourceControlItem` with hardcoded `isSelectedForSync: false` / `operationStatus: 'idle'` / a fresh `resolveSyncAction()` call — it reads the ViewModel's real projection instead, so a queued/running row's diff-tab refresh reflects its actual state. `SourceControlView.loadAndRenderDiff()` (mobile detail view) drops its two-`getState()`-call lookup (needed only to also catch `'synced'` rows) in favor of the unfiltered `getItem()`. No UX change. -**Next:** items 3-5 of the PR2 plan, one at a time, each its own commit — item 3 (centralize Source Control item projection via `SourceControlViewModel.getItem()`) is next up. +**Next:** items 4-5 of the PR2 plan — item 4 (reuse pull orchestration between `SyncManager.pullFile()` and `PullCoordinator`) is next, and per the plan itself, if single-file and batch-pull semantics turn out to differ intentionally, stop rather than force a merge. Item 5 (provider contract cleanup) is small/optional and may be dropped to a follow-up PR if it starts to spread. Below that: the previous "Outstanding Items"/"Verification Evidence" entries track separate, still-open work on PR #129 / `claude/source-control-foundation`, Issue #143, and `claude/fix-source-control-explicit-sync-intent` — not superseded by this entry, carried over from the base branch history. - `npx eslint .` — 0 errors. -- `npx vitest run` — 75 files / 945 tests passed (up from 940; added `tests/settings.test.ts` for the model/helpers split). +- `npx vitest run` — 75 files / 949 tests passed (up from 945; added `SourceControlViewModel.getItem()` tests and a regression test proving the diff-tab refresh uses real selection/operation state, not hardcoded defaults). - `npm run build` (tsc + Obsidian 1.11.0 compat typecheck + esbuild) — passed. ## Outstanding Items From 1c3e459f1ea6beab10c99507f57a7c742b31004a Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 06:53:16 +0000 Subject: [PATCH 07/10] refactor(sync): reuse pull orchestration path SyncManager.pullFile() no longer duplicates PullCoordinator's own no-prefetched-tree classification (exists/local-content/local-sha/baseline computation + SyncPlanner.planFor('pull', ...)) -- including the legacy GitLab revision-keyed baseline correction, which existed identically in both places. Both now go through PullCoordinator.planSingleFile(), a thin wrapper over the existing private planFromRemote(), so single-file and batch pull can no longer silently drift apart on planning semantics. Deliberately left separate (see planSingleFile's doc comment): interactive conflict resolution (single-file pull opens a modal inline; batch pull skips/aggregates conflicts), per-call confirmation (single confirms every call; batch confirms once for the whole plan), and notification (single always reports "up to date"; batch only summarizes). Those are deliberate UX differences, not accidental duplication, so they stay owned by each caller rather than being forced into one shape. Also removes SyncManager's now-dead SyncPlanner instance and its contentsEqual/isBinaryPath/gitBlobSha imports, all of which existed only to duplicate planFromRemote's own logic. Co-Authored-By: Claude Sonnet 5 --- src/logic/sync/PullCoordinator.ts | 19 ++++ src/logic/sync/SyncManager.ts | 27 +---- tests/logic/sync/PullCoordinator.test.ts | 136 +++++++++++++++++++++++ 3 files changed, 161 insertions(+), 21 deletions(-) create mode 100644 tests/logic/sync/PullCoordinator.test.ts diff --git a/src/logic/sync/PullCoordinator.ts b/src/logic/sync/PullCoordinator.ts index 9aec22f..bfe4a67 100644 --- a/src/logic/sync/PullCoordinator.ts +++ b/src/logic/sync/PullCoordinator.ts @@ -62,6 +62,25 @@ export class PullCoordinator { return this.processBatch(files, onProgress, tree, options); } + /** + * Plans one already-fetched remote file exactly like batch pull's own + * per-file classification without a prefetched tree (`planFromRemote`) -- + * the shared decision step `SyncManager.pullFile()` delegates to, so + * single- and batch-pull planning semantics (baseline resolution, + * exists/content/sha comparison) can't silently drift apart. + * + * Deliberately NOT unified: interactive conflict handling ("resolve + * conflict" opens a modal for single-file pull, but is skipped/aggregated + * for batch pull), per-call confirmation (single confirms every call; + * batch confirms once for the whole plan), and notification (single + * always reports "up to date"; batch only summarizes). Those differences + * are deliberate UX, not accidental drift, and stay owned by each caller. + */ + async planSingleFile(file: TFile | string, remote: GitFile): Promise { + const { path, isString } = this.dependencies.scanner.fileInfo(file); + return this.planFromRemote(file, path, isString, remote); + } + async planPullBatch(files: Array, remoteTree?: GitTreeEntry[]): Promise { const tree = remoteTree ? new Map(remoteTree.map(entry => [entry.path, entry])) : undefined; const plan: SyncPlan = { additions: [], modifications: [], deletions: [], moves: [] }; diff --git a/src/logic/sync/SyncManager.ts b/src/logic/sync/SyncManager.ts index f4c5ad9..478e22f 100644 --- a/src/logic/sync/SyncManager.ts +++ b/src/logic/sync/SyncManager.ts @@ -10,8 +10,6 @@ import { isSyncPlanEmpty, } from './types'; import { logger } from '../../utils/logger'; -import { contentsEqual, isBinaryPath } from '../../utils/path'; -import { gitBlobSha } from '../../utils/git-blob-sha'; import { SyncStatusService } from '../sync-status-service'; import { PushExecutor } from './PushExecutor'; import { PullExecutor } from './PullExecutor'; @@ -21,7 +19,6 @@ import { ConflictResolver } from './ConflictResolver'; import { SyncExecutor } from './SyncExecutor'; import { PullCoordinator } from './PullCoordinator'; import { PushCoordinator } from './PushCoordinator'; -import { SyncPlanner } from './SyncPlanner'; import { HeadlessSyncInteraction, type ConflictDiffLoader, @@ -41,7 +38,6 @@ export class SyncManager { private readonly scanner: SyncScanner; private readonly pullCoordinator: PullCoordinator; private readonly pushCoordinator: PushCoordinator; - private readonly planner = new SyncPlanner(); private readonly interaction: SyncInteractionPort; /** Optional progressive +/- diff-stat source handed to the batch conflict modal. */ private diffStatLoader?: ConflictDiffStatLoader; @@ -211,23 +207,12 @@ export class SyncManager { const exists = await this.fileExists(fileOrPath); const localContent = exists ? await this.getFileContent(fileOrPath) : null; - const lastSynced = this.settings.syncMetadata[path]; - const kind = isBinaryPath(path) ? 'binary' : 'text'; - const baseline = lastSynced?.lastSyncedSha === remote.revision ? remote.sha : lastSynced?.lastSyncedSha; - let localSha: string | undefined; - if (localContent !== null) { - localSha = contentsEqual(localContent, remote.content) ? remote.sha : await gitBlobSha(localContent); - } - const decision = this.planner.planFor('pull', { - local: { - path, - exists, - blobSha: localSha, - kind, - }, - remote: { path, repoPath, exists: true, blobSha: remote.sha, kind }, - base: { blobSha: baseline }, - }); + // Shared with batch pull's own no-prefetched-tree classification + // (PullCoordinator.planFromRemote), so single- and batch-pull + // planning semantics can't silently drift apart. Interactive + // conflict handling, confirmation, and notification stay separate + // below -- see PullCoordinator.planSingleFile's doc comment. + const decision = await this.pullCoordinator.planSingleFile(fileOrPath, remote); if (decision.action === 'none') { await this.updateMetadata(path, remote.sha); diff --git a/tests/logic/sync/PullCoordinator.test.ts b/tests/logic/sync/PullCoordinator.test.ts new file mode 100644 index 0000000..515e37b --- /dev/null +++ b/tests/logic/sync/PullCoordinator.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, it, vi } from 'vitest'; +import { PullCoordinator, type PullCoordinatorDependencies } from '../../../src/logic/sync/PullCoordinator'; +import { gitBlobSha } from '../../../src/utils/git-blob-sha'; +import type { GitFile } from '../../../src/services/git-service-interface'; +import type { GitLabFilesPushSettings } from '../../../src/settings'; + +function buildDependencies(overrides: Partial = {}): PullCoordinatorDependencies { + const settings = { + serviceType: 'gitlab', + syncMetadata: {}, + branch: 'main', + vaultFolder: '', + rootPath: '', + } as unknown as GitLabFilesPushSettings; + + return { + gitService: () => ({}) as never, + settings, + scanner: { + fileInfo: (fileOrPath: string) => ({ path: fileOrPath, name: fileOrPath, isString: true }), + toRepoPath: (path: string) => path, + toTreePath: (path: string) => path, + pathExists: vi.fn().mockResolvedValue(false), + indexedFileExists: vi.fn().mockReturnValue(false), + readContent: vi.fn().mockResolvedValue(''), + } as unknown as PullCoordinatorDependencies['scanner'], + executor: { pull: vi.fn().mockResolvedValue(undefined) } as unknown as PullCoordinatorDependencies['executor'], + confirmPlan: vi.fn().mockResolvedValue(true), + updateMetadata: vi.fn().mockResolvedValue(undefined), + migrateBaseline: vi.fn().mockResolvedValue(undefined), + saveSettings: vi.fn().mockResolvedValue(undefined), + notify: vi.fn(), + serviceName: () => 'GitLab', + ...overrides, + }; +} + +describe('PullCoordinator.planSingleFile', () => { + it('plans an addition when the file does not exist locally', async () => { + const deps = buildDependencies(); + const coordinator = new PullCoordinator(deps); + const remote: GitFile = { content: 'remote content', sha: 'remote-sha' }; + + const decision = await coordinator.planSingleFile('new.md', remote); + + expect(decision.action).toBe('pull-create'); + }); + + it('plans none (already up to date) once local content and baseline match the remote blob', async () => { + const localContent = 'same content'; + const sha = await gitBlobSha(localContent); + const deps = buildDependencies({ + settings: { + serviceType: 'gitlab', + syncMetadata: { 'a.md': { lastSyncedSha: sha, lastSyncedAt: 0 } }, + branch: 'main', + vaultFolder: '', + rootPath: '', + } as unknown as GitLabFilesPushSettings, + scanner: { + fileInfo: (fileOrPath: string) => ({ path: fileOrPath, name: fileOrPath, isString: true }), + toRepoPath: (path: string) => path, + toTreePath: (path: string) => path, + pathExists: vi.fn().mockResolvedValue(true), + indexedFileExists: vi.fn().mockReturnValue(true), + readContent: vi.fn().mockResolvedValue(localContent), + } as unknown as PullCoordinatorDependencies['scanner'], + }); + const coordinator = new PullCoordinator(deps); + const remote: GitFile = { content: localContent, sha }; + + const decision = await coordinator.planSingleFile('a.md', remote); + + expect(decision.action).toBe('none'); + }); + + it('resolves a legacy GitLab baseline keyed by revision, the same correction SyncManager.pullFile() used to duplicate', async () => { + // Old GitLab metadata stored the file's `revision` (last_commit_id) as + // lastSyncedSha rather than a blob sha. When the current remote fetch's + // revision still matches that stored value, the true baseline blob is + // the remote's own current sha -- so an unmodified file classifies as + // 'none', not a false-positive conflict/modification. + const content = 'unchanged content'; + const sha = await gitBlobSha(content); + const legacyRevision = 'legacy-commit-id'; + const deps = buildDependencies({ + settings: { + serviceType: 'gitlab', + syncMetadata: { 'a.md': { lastSyncedSha: legacyRevision, lastSyncedAt: 0 } }, + branch: 'main', + vaultFolder: '', + rootPath: '', + } as unknown as GitLabFilesPushSettings, + scanner: { + fileInfo: (fileOrPath: string) => ({ path: fileOrPath, name: fileOrPath, isString: true }), + toRepoPath: (path: string) => path, + toTreePath: (path: string) => path, + pathExists: vi.fn().mockResolvedValue(true), + indexedFileExists: vi.fn().mockReturnValue(true), + readContent: vi.fn().mockResolvedValue(content), + } as unknown as PullCoordinatorDependencies['scanner'], + }); + const coordinator = new PullCoordinator(deps); + const remote: GitFile = { content, sha, revision: legacyRevision }; + + const decision = await coordinator.planSingleFile('a.md', remote); + + expect(decision.action).toBe('none'); + }); + + it('plans resolve-conflict when both sides changed since the baseline', async () => { + const deps = buildDependencies({ + settings: { + serviceType: 'gitlab', + syncMetadata: { 'a.md': { lastSyncedSha: 'base-sha', lastSyncedAt: 0 } }, + branch: 'main', + vaultFolder: '', + rootPath: '', + } as unknown as GitLabFilesPushSettings, + scanner: { + fileInfo: (fileOrPath: string) => ({ path: fileOrPath, name: fileOrPath, isString: true }), + toRepoPath: (path: string) => path, + toTreePath: (path: string) => path, + pathExists: vi.fn().mockResolvedValue(true), + indexedFileExists: vi.fn().mockReturnValue(true), + readContent: vi.fn().mockResolvedValue('local edit'), + } as unknown as PullCoordinatorDependencies['scanner'], + }); + const coordinator = new PullCoordinator(deps); + const remote: GitFile = { content: 'remote edit', sha: 'remote-sha' }; + + const decision = await coordinator.planSingleFile('a.md', remote); + + expect(decision.action).toBe('resolve-conflict'); + }); +}); From c558289cfea668c24526278dc4706460b785d66d Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 06:54:08 +0000 Subject: [PATCH 08/10] docs: record PR2 item 4 (reuse pull orchestration) session progress Co-Authored-By: Claude Sonnet 5 --- progress.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/progress.md b/progress.md index 0a3dcd0..b8ea450 100644 --- a/progress.md +++ b/progress.md @@ -5,17 +5,17 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont ## Current State **Last Updated:** 2026-09-01 -**Active Feature:** PR2 responsibility cleanup, item 3/5 done — centralize Source Control item projection (no tracked issue number; an ad-hoc follow-up plan on top of `origin/1.6.1`, not in `feature_list.json`). +**Active Feature:** PR2 responsibility cleanup, item 4/5 done — reuse pull orchestration path (no tracked issue number; an ad-hoc follow-up plan on top of `origin/1.6.1`, not in `feature_list.json`). **Branch / PR:** `claude/pr2-source-control-boundary`, branched from `origin/1.6.1` (commit `69e5540`). Not yet pushed or opened as a PR. -**Scope (item 3, per the PR2 plan):** Added `SourceControlViewModel.getItem(id)` as the single `SourceControlItem` projection path by id. `SourceControlItemView.refreshOpenDiffTab()` no longer hand-rolls a `SourceControlItem` with hardcoded `isSelectedForSync: false` / `operationStatus: 'idle'` / a fresh `resolveSyncAction()` call — it reads the ViewModel's real projection instead, so a queued/running row's diff-tab refresh reflects its actual state. `SourceControlView.loadAndRenderDiff()` (mobile detail view) drops its two-`getState()`-call lookup (needed only to also catch `'synced'` rows) in favor of the unfiltered `getItem()`. No UX change. +**Scope (item 4, per the PR2 plan):** `SyncManager.pullFile()` no longer duplicates `PullCoordinator`'s no-prefetched-tree classification (exists/local-content/local-sha/baseline + `SyncPlanner.planFor('pull', ...)`, including the legacy GitLab revision-keyed baseline correction) — both now share `PullCoordinator.planSingleFile()`, a thin wrapper over the existing private `planFromRemote()`. Removed `SyncManager`'s now-dead `SyncPlanner` instance and `contentsEqual`/`isBinaryPath`/`gitBlobSha` imports. Deliberately did **not** unify interactive conflict handling, per-call confirmation, or notification — those are deliberate UX differences between single-file and batch pull (documented on `planSingleFile`), not accidental duplication. Also noted but explicitly left alone: `planFromTree` (tree-available path) persists a legacy-baseline correction via `migrateGitLabLegacyBaseline`, while `planFromRemote`/`planSingleFile` (no-tree path) only corrects it ephemerally per-call without persisting — this asymmetry predates this PR and unifying it would be a separate, larger change. -**Next:** items 4-5 of the PR2 plan — item 4 (reuse pull orchestration between `SyncManager.pullFile()` and `PullCoordinator`) is next, and per the plan itself, if single-file and batch-pull semantics turn out to differ intentionally, stop rather than force a merge. Item 5 (provider contract cleanup) is small/optional and may be dropped to a follow-up PR if it starts to spread. +**Next:** item 5 of the PR2 plan (provider contract cleanup — move `ConnectionTestResult` out of `git-service-base.ts`, review `updateConfig(...args: unknown[])`) is small/optional; the plan says drop it to a follow-up PR if it starts to spread. After that, this PR2 branch is otherwise ready to push and open as a PR. Below that: the previous "Outstanding Items"/"Verification Evidence" entries track separate, still-open work on PR #129 / `claude/source-control-foundation`, Issue #143, and `claude/fix-source-control-explicit-sync-intent` — not superseded by this entry, carried over from the base branch history. - `npx eslint .` — 0 errors. -- `npx vitest run` — 75 files / 949 tests passed (up from 945; added `SourceControlViewModel.getItem()` tests and a regression test proving the diff-tab refresh uses real selection/operation state, not hardcoded defaults). +- `npx vitest run` — 76 files / 953 tests passed (up from 949; added `tests/logic/sync/PullCoordinator.test.ts`, the first dedicated test file for `PullCoordinator`, covering `planSingleFile`'s addition/none/legacy-baseline/conflict cases). - `npm run build` (tsc + Obsidian 1.11.0 compat typecheck + esbuild) — passed. ## Outstanding Items From dae020d364498ee90c71964d57dec03a818f5d90 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 07:22:16 +0000 Subject: [PATCH 09/10] refactor(services): move ConnectionTestResult to the interface module ConnectionTestResult is part of GitServiceInterface's contract (testConnection's return type), not an implementation detail of BaseGitService, so it now lives in git-service-interface.ts alongside the interface that references it. git-service-base.ts imports it back for its own abstract testConnection signature. Left updateConfig(...args: unknown[]) as-is: every call site invokes it on the concrete service class, never through GitServiceInterface, so the loose signature isn't causing an actual type-safety gap. Converting it to a typed discriminated union would require reshaping the interface, all three services' updateConfig bodies, and all three main.ts call sites for no functional benefit -- deferred rather than folded into this cleanup. Co-Authored-By: Claude Sonnet 5 --- src/main.ts | 3 +-- src/services/git-service-base.ts | 11 +---------- src/services/git-service-interface.ts | 9 ++++++++- src/services/gitea-service.ts | 3 ++- src/services/github-service.ts | 3 ++- src/services/gitlab-service.ts | 3 ++- src/ui/settings/GitLabSyncSettingTab.ts | 2 +- tests/ui/SettingsConnectionStatus.test.ts | 2 +- 8 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/main.ts b/src/main.ts index 32ccc9f..2a5c6d4 100644 --- a/src/main.ts +++ b/src/main.ts @@ -3,8 +3,7 @@ import { DEFAULT_SETTINGS, GitLabFilesPushSettings, GitLabSyncSettingTab, getSer import { GitLabService } from './services/gitlab-service'; 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 { ConnectionTestResult, GitServiceInterface, GitTreeEntry } from './services/git-service-interface'; 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'; diff --git a/src/services/git-service-base.ts b/src/services/git-service-base.ts index 1442351..693ad05 100644 --- a/src/services/git-service-base.ts +++ b/src/services/git-service-base.ts @@ -1,6 +1,6 @@ import { requestUrl, RequestUrlResponse } from 'obsidian'; import { logger } from '../utils/logger'; -import { GitTreeEntry } from './git-service-interface'; +import { ConnectionTestResult, GitTreeEntry } from './git-service-interface'; import { isBinaryPath } from '../utils/path'; export interface GitFile { @@ -48,15 +48,6 @@ export interface GitLabTreeItem { id?: string; } -export interface ConnectionTestResult { - /** Whether the repository/project itself was reachable with the given credentials. */ - repoOk: boolean; - /** Whether the configured branch was found. Only meaningful when repoOk is true. */ - branchOk: boolean; - /** Populated when repoOk is false, describing the repo-level failure. */ - error?: string; -} - /** Max files per single batch-commit call. Guards against oversized request * bodies / provider payload limits when a vault has thousands of files. */ export const MAX_BATCH_PUSH_SIZE = 200; diff --git a/src/services/git-service-interface.ts b/src/services/git-service-interface.ts index 92bb758..d46929c 100644 --- a/src/services/git-service-interface.ts +++ b/src/services/git-service-interface.ts @@ -1,4 +1,11 @@ -import { ConnectionTestResult } from './git-service-base'; +export interface ConnectionTestResult { + /** Whether the repository/project itself was reachable with the given credentials. */ + repoOk: boolean; + /** Whether the configured branch was found. Only meaningful when repoOk is true. */ + branchOk: boolean; + /** Populated when repoOk is false, describing the repo-level failure. */ + error?: string; +} export interface GitFile { content: string | ArrayBuffer; diff --git a/src/services/gitea-service.ts b/src/services/gitea-service.ts index 7cca54a..2c75c6a 100644 --- a/src/services/gitea-service.ts +++ b/src/services/gitea-service.ts @@ -1,5 +1,6 @@ import { GitServiceInterface, GitTreeEntry, BatchPushItem, BatchPushResult, BatchCommitPlan } from './git-service-interface'; -import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE } from './git-service-base'; +import { BaseGitService, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE } from './git-service-base'; +import { ConnectionTestResult } from './git-service-interface'; /** One entry in a Gitea "change multiple files" request. */ interface GiteaChangeFileOperation { diff --git a/src/services/github-service.ts b/src/services/github-service.ts index 85eab91..5b76bc6 100644 --- a/src/services/github-service.ts +++ b/src/services/github-service.ts @@ -1,5 +1,6 @@ import { GitServiceInterface, GitTreeEntry, BatchPushItem, BatchPushResult, BatchCommitPlan } from './git-service-interface'; -import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE, BLOB_CREATE_CONCURRENCY } from './git-service-base'; +import { BaseGitService, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE, BLOB_CREATE_CONCURRENCY } from './git-service-base'; +import { ConnectionTestResult } from './git-service-interface'; import { PushTimingCollector, PushTimingHandler, PushTimingRecord } from './push-timing'; /** diff --git a/src/services/gitlab-service.ts b/src/services/gitlab-service.ts index f71aef1..a00568d 100644 --- a/src/services/gitlab-service.ts +++ b/src/services/gitlab-service.ts @@ -1,5 +1,6 @@ import { GitServiceInterface, GitTreeEntry, BatchPushItem, BatchPushResult, BatchCommitPlan } from './git-service-interface'; -import { BaseGitService, ConnectionTestResult, GitFile, GitLabFileResponse, GitLabTreeItem, GIT_SYMLINK_MODE } from './git-service-base'; +import { BaseGitService, GitFile, GitLabFileResponse, GitLabTreeItem, GIT_SYMLINK_MODE } from './git-service-base'; +import { ConnectionTestResult } from './git-service-interface'; import { isBinaryPath } from '../utils/path'; export class GitLabService extends BaseGitService implements GitServiceInterface { diff --git a/src/ui/settings/GitLabSyncSettingTab.ts b/src/ui/settings/GitLabSyncSettingTab.ts index 04df676..462a5ba 100644 --- a/src/ui/settings/GitLabSyncSettingTab.ts +++ b/src/ui/settings/GitLabSyncSettingTab.ts @@ -5,7 +5,7 @@ import type { ConnectionStatus } from '../../main'; // that unrelated widget's needs would leak scope into this PR; narrowing // RemoteFolderSuggest itself is a separate cleanup, not part of this one. import type GitLabFilesPush from '../../main'; -import type { ConnectionTestResult } from '../../services/git-service-base'; +import type { ConnectionTestResult } from '../../services/git-service-interface'; import { FolderSuggest } from '../FolderSuggest'; import { RemoteFolderSuggest } from '../RemoteFolderSuggest'; import { WhatsNewModal } from '../WhatsNewModal'; diff --git a/tests/ui/SettingsConnectionStatus.test.ts b/tests/ui/SettingsConnectionStatus.test.ts index 01ea074..8508b41 100644 --- a/tests/ui/SettingsConnectionStatus.test.ts +++ b/tests/ui/SettingsConnectionStatus.test.ts @@ -2,7 +2,7 @@ import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vite import { App } from 'obsidian'; import { DEFAULT_SETTINGS, GitLabSyncSettingTab } from '../../src/settings'; import GitLabFilesPush from '../../src/main'; -import type { ConnectionTestResult } from '../../src/services/git-service-base'; +import type { ConnectionTestResult } from '../../src/services/git-service-interface'; import { createContainer, setupObsidianDOM } from './setup-dom'; vi.mock('../../src/main', () => ({ From 7ae0e3078294a59a88a8924a1f6ea35e6758ed07 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 07:22:33 +0000 Subject: [PATCH 10/10] docs: record PR2 item 5 (provider contract cleanup) session progress Co-Authored-By: Claude Sonnet 5 --- progress.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/progress.md b/progress.md index b8ea450..f0a6c22 100644 --- a/progress.md +++ b/progress.md @@ -5,17 +5,17 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont ## Current State **Last Updated:** 2026-09-01 -**Active Feature:** PR2 responsibility cleanup, item 4/5 done — reuse pull orchestration path (no tracked issue number; an ad-hoc follow-up plan on top of `origin/1.6.1`, not in `feature_list.json`). -**Branch / PR:** `claude/pr2-source-control-boundary`, branched from `origin/1.6.1` (commit `69e5540`). Not yet pushed or opened as a PR. +**Active Feature:** PR2 responsibility cleanup, item 5 done — provider contract cleanup, partial (no tracked issue number; an ad-hoc follow-up plan on top of `origin/1.6.1`, not in `feature_list.json`). +**Branch / PR:** `claude/pr2-source-control-boundary`, branched from `origin/1.6.1` (commit `69e5540`). Pushed; opened as [PR #154](https://github.com/firstsun-dev/git-files-sync/pull/154) against `1.6.1` (covers items 1-4; item 5 below lands as a follow-up commit on the same branch/PR). -**Scope (item 4, per the PR2 plan):** `SyncManager.pullFile()` no longer duplicates `PullCoordinator`'s no-prefetched-tree classification (exists/local-content/local-sha/baseline + `SyncPlanner.planFor('pull', ...)`, including the legacy GitLab revision-keyed baseline correction) — both now share `PullCoordinator.planSingleFile()`, a thin wrapper over the existing private `planFromRemote()`. Removed `SyncManager`'s now-dead `SyncPlanner` instance and `contentsEqual`/`isBinaryPath`/`gitBlobSha` imports. Deliberately did **not** unify interactive conflict handling, per-call confirmation, or notification — those are deliberate UX differences between single-file and batch pull (documented on `planSingleFile`), not accidental duplication. Also noted but explicitly left alone: `planFromTree` (tree-available path) persists a legacy-baseline correction via `migrateGitLabLegacyBaseline`, while `planFromRemote`/`planSingleFile` (no-tree path) only corrects it ephemerally per-call without persisting — this asymmetry predates this PR and unifying it would be a separate, larger change. +**Scope (item 5, per the PR2 plan):** Moved `ConnectionTestResult` out of `git-service-base.ts` into `git-service-interface.ts` — it's a contract type consumed by `GitServiceInterface.testConnection`, so it belongs with the interface, not the base implementation class. `git-service-base.ts` now imports it back for its own `abstract testConnection` signature; `github-service.ts`/`gitlab-service.ts`/`gitea-service.ts`/`main.ts`/`GitLabSyncSettingTab.ts`/`tests/ui/SettingsConnectionStatus.test.ts` updated to import from the new location. Reviewed `updateConfig(...args: unknown[])` on `GitServiceInterface` per the plan's ask, but did **not** convert it to a typed discriminated union: every actual call site (`main.ts` `initializeGitService()`, 3 branches) already calls `updateConfig` on the concrete class (`GitLabService`/`GiteaService`/`GitHubService`), never through the loose interface type, so the untyped signature isn't causing a real type-safety gap today. A discriminated union would mean reshaping the interface, all three services' `updateConfig` bodies, and all three `main.ts` call sites into config-object form for no functional benefit — exactly the "touches too much, leave for later" case the plan calls out, so left as-is. -**Next:** item 5 of the PR2 plan (provider contract cleanup — move `ConnectionTestResult` out of `git-service-base.ts`, review `updateConfig(...args: unknown[])`) is small/optional; the plan says drop it to a follow-up PR if it starts to spread. After that, this PR2 branch is otherwise ready to push and open as a PR. +**Next:** PR2 plan is now fully worked through (items 1-5). Nothing further planned here; watch PR #154 for review feedback. Below that: the previous "Outstanding Items"/"Verification Evidence" entries track separate, still-open work on PR #129 / `claude/source-control-foundation`, Issue #143, and `claude/fix-source-control-explicit-sync-intent` — not superseded by this entry, carried over from the base branch history. - `npx eslint .` — 0 errors. -- `npx vitest run` — 76 files / 953 tests passed (up from 949; added `tests/logic/sync/PullCoordinator.test.ts`, the first dedicated test file for `PullCoordinator`, covering `planSingleFile`'s addition/none/legacy-baseline/conflict cases). +- `npx vitest run` — 76 files / 953 tests passed (unchanged count; pure type-relocation, no new tests needed). - `npm run build` (tsc + Obsidian 1.11.0 compat typecheck + esbuild) — passed. ## Outstanding Items