From a1f01b3394bc49fc1ba0ad3df101769d2cf412fb Mon Sep 17 00:00:00 2001 From: tianyao Date: Wed, 2 Sep 2026 02:19:24 +0000 Subject: [PATCH 1/7] fix(ui): unify mobile Source Control row density across Queue, List, and Tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Queue rows already set the compact mobile row baseline; List and Tree file rows, plus tree folder rows, now read the same shared --scv-mobile-row-* custom properties instead of drifting independently. List mode's folder-path suffix now yields horizontal space before the filename (disproportionate flex-shrink) so long paths ellipsis first and the row never wraps to a second line. Also normalizes the Queue→Repository vertical gap and both sections' header→first-row padding to the same values. CSS-only; no changes to SourceControlViewModel, selection semantics, sync behavior, tree shaping, scroll persistence, or Queue/Repository responsibilities. --- styles.css | 44 +++++++-- .../source-control/SourceControlView.test.ts | 90 +++++++++++++++++++ 2 files changed, 128 insertions(+), 6 deletions(-) diff --git a/styles.css b/styles.css index 756374f..bd5b5b7 100644 --- a/styles.css +++ b/styles.css @@ -41,6 +41,12 @@ overflow: hidden; padding: 0; container-type: inline-size; + /* Shared mobile row-density baseline: the Sync Queue's existing compact + row (unchanged) is the visual reference. Repository List and Tree file + rows, plus tree folder rows, read these same values instead of each + maintaining their own effective height. */ + --scv-mobile-row-min-height: 30px; + --scv-mobile-row-padding: 6px 10px; } .scv-header { @@ -312,7 +318,10 @@ body.is-mobile .scv-view-toggle-label { display: none; } /* ── Selected-for-sync workspace ───────────────────────────── */ .scv-selected-section { flex-shrink: 0; - margin: 6px 8px 4px 8px; + /* No bottom margin: Repository Changes' own header already contributes + the same 6px top padding used above the Queue, so Queue→Repository + stays a single 6px gap instead of stacking on top of it. */ + margin: 6px 8px 0 8px; padding: 6px 0; border-radius: var(--radius-m); background: var(--background-secondary); @@ -324,7 +333,9 @@ body.is-mobile .scv-view-toggle-label { display: none; } display: flex; align-items: center; gap: 6px; - padding: 2px 10px 6px 10px; + /* Bottom padding matches .scv-repository-header's, so both sections' + header→first-row gap reads the same. */ + padding: 2px 10px 4px 10px; color: var(--text-muted); font-size: 0.74em; font-weight: 600; @@ -533,6 +544,8 @@ body.is-mobile .scv-view-toggle-label { display: none; } .scv-change-item { display: flex; align-items: center; + flex-wrap: nowrap; + overflow: hidden; gap: 7px; min-height: 30px; padding: 5px 12px 5px 8px; @@ -600,11 +613,15 @@ body.is-mobile .scv-view-toggle-label { display: none; } } /* List-view variant: the name shrinks to its content so the folder path - suffix can sit on the right, disambiguating flat rows without nesting. */ -.scv-change-item-list .scv-change-name { flex: 0 1 auto; } + suffix can sit on the right, disambiguating flat rows without nesting. + flex-shrink stays low relative to .scv-change-path's (below) so the path + yields space first when the row is tight -- the filename stays readable + longest, both ellipsis rather than wrap. */ +.scv-change-item-list .scv-change-name { flex: 0 1 auto; flex-shrink: 1; min-width: 0; } .scv-change-path { - flex: 1 1 auto; + flex: 0 1 auto; + flex-shrink: 20; min-width: 0; margin-left: auto; overflow: hidden; @@ -1072,10 +1089,25 @@ body.is-mobile .scv-change-menu { min-width: 28px; min-height: 28px; } } /* ── Mobile adjustments ─────────────────────────────────────────── */ +/* Row-density baseline (Sync Queue/Repository List/Repository Tree file and + folder rows all read the same var(--scv-mobile-row-*) values -- see + .scv-root) applies both by platform (phone/tablet, body.is-mobile) and by + narrow panel width (@container), so a compact row shows up whichever + condition made the layout tight. */ +body.is-mobile .scv-change-item, +body.is-mobile .scv-tree-folder-row { + min-height: var(--scv-mobile-row-min-height); + padding: var(--scv-mobile-row-padding); +} + @container (max-width: 480px) { .scv-filter-menu { padding: 6px 8px; gap: 3px; } .scv-filter-option { padding: 4px 8px; font-size: 0.76em; } - .scv-change-item { padding: 6px 10px; } + .scv-change-item, + .scv-tree-folder-row { + min-height: var(--scv-mobile-row-min-height); + padding: var(--scv-mobile-row-padding); + } .scv-change-name { font-size: 0.76em; } } diff --git a/tests/ui/source-control/SourceControlView.test.ts b/tests/ui/source-control/SourceControlView.test.ts index df0fa0e..a98321b 100644 --- a/tests/ui/source-control/SourceControlView.test.ts +++ b/tests/ui/source-control/SourceControlView.test.ts @@ -1588,6 +1588,96 @@ describe('SourceControlView', () => { expect(onSync).toHaveBeenCalledWith([{ changeId: toChangeId('c-1'), action: undefined }]); }); + + describe('row density parity (Queue / Repository Tree / Repository List)', () => { + it('renders Queue, Tree, and List file rows all as .scv-change-item so they share one CSS density baseline', () => { + Platform.isMobile = true; + const { view, selection } = buildView([ + { id: toChangeId('c-1'), path: 'notes/a.md', kind: 'local-only' }, + { id: toChangeId('c-2'), path: 'notes/b.md', kind: 'local-only' }, + ]); + selection.selectForSync(toChangeId('c-1')); + view.render(container); + + const queueRow = container.querySelector('.scv-selected-section .scv-change-item'); + const treeRow = container.querySelector('.scv-changes-tree .scv-change-item'); + expect(queueRow).not.toBeNull(); + expect(treeRow).not.toBeNull(); + expect(queueRow?.classList.contains('scv-change-item')).toBe(true); + expect(treeRow?.classList.contains('scv-change-item')).toBe(true); + // Neither carries the list-only variant class -- both stay on + // the shared bare-row density baseline. + expect(queueRow?.classList.contains('scv-change-item-list')).toBe(false); + expect(treeRow?.classList.contains('scv-change-item-list')).toBe(false); + + (container.querySelector('.scv-view-toggle-btn[data-view="list"]') as HTMLButtonElement).click(); + const listRow = container.querySelector('.scv-changes-tree .scv-change-item'); + expect(listRow?.classList.contains('scv-change-item')).toBe(true); + expect(listRow?.classList.contains('scv-change-item-list')).toBe(true); + }); + + it('List mode keeps filename, folder path, diff stat, and the row menu inside one single-row element', () => { + Platform.isMobile = true; + const { view } = buildView([ + { id: toChangeId('c-1'), path: 'deep/nested/folder/report.md', kind: 'local-modified' }, + ]); + view.render(container); + (container.querySelector('.scv-view-toggle-btn[data-view="list"]') as HTMLButtonElement).click(); + + const rows = container.querySelectorAll('.scv-changes-tree .scv-change-item'); + // Exactly one row for the one change -- no extra wrapper rows + // that would indicate the content spilled onto a second line. + expect(rows).toHaveLength(1); + const row = rows[0] as HTMLElement; + expect(row.querySelector('.scv-change-name-text')?.textContent).toBe('report.md'); + expect(row.querySelector('.scv-change-path')?.textContent).toBe('deep/nested/folder'); + expect(row.querySelector('.scv-change-menu')).not.toBeNull(); + }); + + it('Tree mode omits the folder-path suffix (folders already convey location)', () => { + Platform.isMobile = true; + const { view } = buildView([ + { id: toChangeId('c-1'), path: 'deep/nested/folder/report.md', kind: 'local-modified' }, + ]); + view.render(container); + + const row = container.querySelector('.scv-changes-tree .scv-change-item') as HTMLElement; + expect(row.querySelector('.scv-change-path')).toBeNull(); + }); + + it('renders tree folder rows with the shared .scv-tree-folder-row class alongside file rows', () => { + Platform.isMobile = true; + const { view } = buildView([ + { id: toChangeId('c-1'), path: 'notes/a.md', kind: 'local-only' }, + { id: toChangeId('c-2'), path: 'notes/b.md', kind: 'local-only' }, + ]); + view.render(container); + + expect(container.querySelector('.scv-tree-folder-row')).not.toBeNull(); + expect(container.querySelector('.scv-tree-folder-row .scv-change-item')).toBeNull(); + }); + }); + + it('renders the mobile sync bar exactly once, as a sibling after the scrollable body (space reserved once, not overlaid)', () => { + Platform.isMobile = true; + const { view, selection } = buildView([ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }, + ]); + selection.selectForSync(toChangeId('c-1')); + view.render(container); + + const bars = container.querySelectorAll('.scv-mobile-sync-bar'); + expect(bars).toHaveLength(1); + const bar = container.querySelector('.scv-mobile-sync-bar') as HTMLElement; + const body = container.querySelector('.scv-body'); + expect(body).not.toBeNull(); + // Sibling of .scv-body (same parent), not nested inside it or + // inside the independently-scrolling changes region -- so it sits + // in normal flow and its height is reserved exactly once. + expect(bar.parentElement).toBe(body?.parentElement); + expect(body?.contains(bar)).toBe(false); + expect(container.querySelector('.scv-changes-region')?.contains(bar)).toBe(false); + }); }); describe('header info strip', () => { From bd12bbf549d918d4c1cdb7c5d619a1da23dbf160 Mon Sep 17 00:00:00 2001 From: Tianyao Date: Sat, 12 Sep 2026 21:52:17 +0800 Subject: [PATCH 2/7] feat(sync): add automatic scheduled sync --- README.md | 8 +- docs/architecture.md | 34 +++- docs/source-control.md | 20 +++ .../provider/suites/sync-manager.e2e.test.ts | 3 + .../provider/support/sync-manager-fixture.ts | 3 + eslint.config.mts | 1 + feature_list.json | 10 +- progress.md | 23 +-- session-handoff.md | 43 +++-- src/changelog/1.7.0/index.ts | 43 +++++ src/changelog/index.ts | 2 + src/i18n/locales/en.ts | 7 + src/i18n/locales/zh-cn.ts | 7 + src/i18n/locales/zh-tw.ts | 7 + .../source-control/AutomaticSyncService.ts | 74 +++++++++ .../SourceControlActionService.ts | 98 ++++++++---- .../source-control/SyncExecutionGuard.ts | 53 +++++++ .../source-control/SyncIntentExecutor.ts | 87 ++++++++-- src/logic/sync/PushCoordinator.ts | 34 ++++ src/logic/sync/SyncManager.ts | 5 +- src/logic/sync/SyncWorkspace.ts | 17 +- src/main.ts | 47 ++++-- src/runtime/AutomaticSyncScheduler.ts | 68 ++++++++ src/runtime/createSyncRuntime.ts | 10 ++ src/settings/helpers.ts | 23 +++ src/settings/model.ts | 9 ++ src/ui/settings/GitLabSyncSettingTab.ts | 41 ++++- tests/changelog.test.ts | 38 +++++ tests/i18n/index.test.ts | 43 +++++ .../AutomaticSyncService.test.ts | 145 +++++++++++++++++ .../SourceControlActionService.test.ts | 132 ++++++++++++++++ .../source-control/SyncExecutionGuard.test.ts | 50 ++++++ tests/logic/sync-manager-mapping.test.ts | 3 + tests/logic/sync-manager.test.ts | 5 +- tests/logic/sync/PushCoordinator.test.ts | 69 +++++++- tests/main.test.ts | 65 ++++++++ tests/runtime/AutomaticSyncScheduler.test.ts | 129 +++++++++++++++ tests/runtime/createSyncRuntime.test.ts | 22 +++ tests/settings.test.ts | 45 ++++++ tests/setup.ts | 96 ++++++++++-- tests/ui/SettingsAutomaticSync.test.ts | 148 ++++++++++++++++++ tests/ui/SettingsConnectionStatus.test.ts | 15 ++ 42 files changed, 1672 insertions(+), 110 deletions(-) create mode 100644 src/changelog/1.7.0/index.ts create mode 100644 src/logic/source-control/AutomaticSyncService.ts create mode 100644 src/logic/source-control/SyncExecutionGuard.ts create mode 100644 src/runtime/AutomaticSyncScheduler.ts create mode 100644 tests/logic/source-control/AutomaticSyncService.test.ts create mode 100644 tests/logic/source-control/SyncExecutionGuard.test.ts create mode 100644 tests/runtime/AutomaticSyncScheduler.test.ts create mode 100644 tests/ui/SettingsAutomaticSync.test.ts diff --git a/README.md b/README.md index b54870f..2265082 100644 --- a/README.md +++ b/README.md @@ -129,7 +129,13 @@ When both sides changed, Git File Sync keeps the conflict explicit. Choose **Kee | **GitLab** | Token, project ID, base URL | `read_repository`, `write_repository` | | **Gitea** | Token, owner, repository, base URL | `write:repository` on Gitea 1.19+ | -Other settings include language, branch, repository root path, vault-folder scope, startup refresh, ignore patterns, and symbolic-link handling. See [Symbolic link handling](docs/symlink-handling.md) for details. +Other settings include language, branch, repository root path, vault-folder scope, ignore patterns, and symbolic-link handling. See [Symbolic link handling](docs/symlink-handling.md) for details. + +### Automatic sync + +Automatic sync is **off by default**. When enabled, Git File Sync refreshes local and remote state on a configurable interval (minimum 1 minute) and applies the same default action the manual **Sync** button would for each pending change — push, pull, or remote delete — through the normal Source Control pipeline. An optional **Sync on startup** runs one automatic pass after Obsidian finishes loading without opening the Source Control view. + +Files that need manual conflict resolution are always skipped: they stay visible as conflicts while unrelated safe changes continue to sync. Automatic runs never show confirmation or conflict dialogs, stay silent on success, and skip a tick when another sync is already running. **Refresh status on startup** is a separate setting that only refreshes the Source Control status view. > **Security:** scope tokens to the smallest possible repository access and permissions, set an expiration where possible, and never place a token inside a note that may be synced. Revoke and rotate a token immediately if it may have been exposed. diff --git a/docs/architecture.md b/docs/architecture.md index bd5dcef..3d61775 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -34,7 +34,10 @@ The dependency direction should normally flow downward. Results and state flow b | Application | `ChangeActionPolicy` | allowed/default action for each change kind | ViewModel, selection reconciliation, intent execution | UI rendering, network calls | | Application | `SourceControlViewModel` | read-only projection of application state for UI | repository, selection, operation/refresh state | side effects, provider calls, filesystem writes | | Application | `SourceControlActionService` | stable UI-facing facade for immediate Source Control commands | `SyncWorkspace`, `SyncIntentExecutor` | provider-specific logic, duplicated sync planning | -| Application | `SyncIntentExecutor` | one Sync Queue workflow: resolve intent, plan, confirm, execute, aggregate | repository, action policy, `SyncWorkspace`, notifier | UI DOM, provider API implementation | +| Application | `SyncIntentExecutor` | one Sync Queue workflow: resolve intent, plan, confirm, execute, aggregate; selects interactive vs background execution policy per run | repository, action policy, `SyncWorkspace`, notifier, `SyncExecutionGuard` | UI DOM, provider API implementation | +| Application | `AutomaticSyncService` | one scheduled automatic sync run: refresh, read repository, build default intents, execute in background, refresh again | `SyncWorkspace`, `ChangeRepository`, `SourceControlActionService` | timer/scheduling mechanics, UI rendering, classification rules | +| Application | `SyncExecutionGuard` | application-level serialization of provider mutations (manual waits, automatic try-acquires) | `SyncIntentExecutor`, `SourceControlActionService` | provider calls, scheduling | +| Plugin runtime | `AutomaticSyncScheduler` (`src/runtime/AutomaticSyncScheduler.ts`) | the automatic-sync interval timer and its lifecycle registration | settings, `AutomaticSyncService`, Obsidian `registerInterval` | sync execution, change classification | | Boundary | `SyncWorkspace` | application-to-sync execution boundary | `SyncManager`, refresh service, diff service | Source Control rendering | | Sync domain | `SyncManager` | compatibility/domain facade for sync operations | coordinators, executors, metadata/status services | Source Control UI state | | Sync domain | `PushCoordinator` | batch push use case including planning/conflict/review/commit coordination | planner, conflict resolver, push executor | Source Control selection state | @@ -108,15 +111,37 @@ SyncIntentExecutor ↓ resolve current ChangeId + revalidate explicit action ↓ -build one merged Sync Plan +build one merged Sync plan ↓ -confirm once +execution policy (interactive: confirm once / background: auto-accept, skip conflicts) ↓ SyncWorkspace ├─ remote mutation bucket (max one provider batch) └─ local pull bucket ``` +### Automatic sync flow + +```text +AutomaticSyncScheduler (timer, plugin runtime) + ↓ +AutomaticSyncService.runOnce() + ↓ +refresh authoritative local + remote state + ↓ +ChangeRepository → exclude synced/conflict → default intents via ChangeActionPolicy + ↓ +SourceControlActionService.sync(intents, 'background') + ↓ +SyncIntentExecutor (skip-conflict planning, no confirmation) + ↓ +SyncWorkspace → Sync domain → provider + ↓ +refresh status again +``` + +Automatic sync reuses the same application → `SyncWorkspace` → domain → provider path as manual Sync. It does not own classification, rename detection, action routing, conflict algorithms, push/pull planning, or provider mutation logic, and it never calls a concrete provider service directly. + ## 4. Architecture rules ### MUST @@ -129,6 +154,9 @@ SyncWorkspace - One Sync Queue action must produce one merged review/confirmation flow. - Remote mutations from one Sync Queue execution must be grouped into at most one provider mutation batch when supported by the current workflow. - Existing compatibility identifiers such as `sync-status-view` and `open-sync-status` must be preserved unless a migration explicitly removes them. +- Automatic sync must execute through `SourceControlActionService`/`SyncIntentExecutor`/`SyncWorkspace`, never a second sync engine. +- Manual and automatic provider mutations must be serialized (`SyncExecutionGuard`); automatic work skips a tick rather than queuing when the path is busy. +- Interactive vs background behavior must be an explicit per-execution policy (`SyncExecutionMode` / `PushConflictBehavior`), not a collection of independent booleans. ### MUST NOT diff --git a/docs/source-control.md b/docs/source-control.md index ff1058e..6eb1f25 100644 --- a/docs/source-control.md +++ b/docs/source-control.md @@ -40,6 +40,26 @@ SourceControlItemView talks directly to a provider or bypasses the workspace to reach sync-domain coordinators/executors. +## Automatic sync + +`AutomaticSyncService` is the application-level use case for one automatic run +(refresh → read `ChangeRepository` → default intents → execute in background → +refresh). It reuses this same call chain and the same `SyncWorkspace` boundary; +it never reaches a provider or coordinator directly. + +- Manual `sync()` stays `interactive`: one merged plan, one confirmation, batch + conflict interaction, and a result notice. +- Automatic runs pass `background`: the same plan is built and validated, but + conflicts are skipped (`PushConflictBehavior = 'skip'`) instead of prompting, + the final plan is auto-accepted, and success is silent. +- `PushCoordinator` stays UI-free: it only receives the small conflict-behavior + switch, never Obsidian or an execution mode. +- Conflicting paths left out of the plan are never marked as operation success; + they remain conflicts after the final refresh. +- `SyncExecutionGuard` serializes provider mutations: manual work waits, + automatic work try-acquires and skips when busy. Timer mechanics live in + `AutomaticSyncScheduler` (plugin runtime), not in the service. + ## Sync Queue invariant One Sync click produces one explicit-intent workflow. Requested action diff --git a/e2e-tests/provider/suites/sync-manager.e2e.test.ts b/e2e-tests/provider/suites/sync-manager.e2e.test.ts index 88b591f..d668c88 100644 --- a/e2e-tests/provider/suites/sync-manager.e2e.test.ts +++ b/e2e-tests/provider/suites/sync-manager.e2e.test.ts @@ -51,6 +51,9 @@ function makeSettings(branch: string): GitLabFilesPushSettings { bannerDismissedVersion: '', language: 'system', autoRefreshOnStartup: true, + automaticSyncEnabled: false, + automaticSyncIntervalMinutes: 5, + automaticSyncOnStartup: false, }; } diff --git a/e2e-tests/provider/support/sync-manager-fixture.ts b/e2e-tests/provider/support/sync-manager-fixture.ts index 03880fb..1286669 100644 --- a/e2e-tests/provider/support/sync-manager-fixture.ts +++ b/e2e-tests/provider/support/sync-manager-fixture.ts @@ -130,6 +130,9 @@ export async function createSyncManagerFixture(options: SyncManagerFixtureOption bannerDismissedVersion: '', language: 'system', autoRefreshOnStartup: true, + automaticSyncEnabled: false, + automaticSyncIntervalMinutes: 5, + automaticSyncOnStartup: false, }; } diff --git a/eslint.config.mts b/eslint.config.mts index c529bbe..08e7204 100644 --- a/eslint.config.mts +++ b/eslint.config.mts @@ -145,6 +145,7 @@ export default tseslint.config( files: [ "tests/ui/SettingsConnectionStatus.test.ts", "tests/ui/SettingsObsidian113Compatibility.test.ts", + "tests/ui/SettingsAutomaticSync.test.ts", ], rules: { "@typescript-eslint/no-deprecated": "off", diff --git a/feature_list.json b/feature_list.json index 904c01f..b5879a3 100644 --- a/feature_list.json +++ b/feature_list.json @@ -1,7 +1,15 @@ { "_note": "GitHub Issues (firstsun-dev/git-files-sync, Project #6) is the source of truth for the full backlog and priority/estimate fields. This file mirrors only the active feature and the next few candidates so an agent session has a local, offline checkpoint — sync it against `gh issue list --repo firstsun-dev/git-files-sync --state open` at the start of a session rather than treating it as authoritative.", - "_lastSync": "2026-08-31: Synced against open GitHub issues; issue #143 is active locally, while #139 awaits review on PR #140.", + "_lastSync": "2026-09-12: Issue #141 (Automatic Syncing) implemented on claude/automatic-sync-141; #143/#139 entries below are carried over from the base branch history.", "features": [ + { + "id": "feat-029", + "name": "feat(sync): automatic scheduled sync (issue #141)", + "description": "Scheduled + optional startup automatic sync that reuses the Source Control execution path in a background policy, skipping conflicts safely.", + "dependencies": ["claude/mobile-source-control-density (PR #156)"], + "status": "in-review", + "evidence": "Branch claude/automatic-sync-141; eslint 0 errors, build passed, vitest 80 files / 1010 tests passed; stacked PR pending." + }, { "id": "feat-027", "name": "test(e2e): run disposable Gitea safely in local and CI environments (issue #139)", diff --git a/progress.md b/progress.md index f0a6c22..a8c305d 100644 --- a/progress.md +++ b/progress.md @@ -4,20 +4,16 @@ 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 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). +**Last Updated:** 2026-09-12 +**Active Feature:** Issue #141 — Automatic Syncing (v1.7.0). Implementation complete, verified locally; PR pending. +**Branch / PR:** `claude/automatic-sync-141`, a child branch of `claude/mobile-source-control-density` (PR #156, still open). The #141 PR is stacked on that baseline; retarget to `main` only if #156 merges first. PR title: `feat(sync): add automatic scheduled sync` (semantic-release owns the 1.7.0 bump). -**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. +**What landed (#141):** persisted `automaticSyncEnabled` / `automaticSyncIntervalMinutes` / `automaticSyncOnStartup` (defaults OFF / 5 / OFF, interval min 1); settings UI rows distinct from the existing `autoRefreshOnStartup`; EN/zh-TW/zh-CN strings; `AutomaticSyncService` (refresh → repository → default intents → background execute → refresh) wired through `createSyncRuntime`; `AutomaticSyncScheduler` in plugin runtime; `SyncExecutionMode` per-execution policy with `PushConflictBehavior = 'skip'` at the `PushCoordinator` planning boundary; `SyncExecutionGuard` serialization; startup sync that never opens Source Control and supersedes the legacy startup refresh; hand-curated 1.7.0 What's New entry. -**Next:** PR2 plan is now fully worked through (items 1-5). Nothing further planned here; watch PR #154 for review feedback. +**Next:** open the stacked PR (body includes `Closes #141`), then monitor CI. Manual Obsidian verification not performed in this environment (no executable Obsidian runtime) — checklist is in the PR body. 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 (unchanged count; pure type-relocation, no new tests needed). -- `npm run build` (tsc + Obsidian 1.11.0 compat typecheck + esbuild) — passed. - ## Outstanding Items 1. Run `npm run test:e2e -- --provider github`, `gitlab`, and `gitea` with provisioned credentials; verify mixed-100 remains under 120s (target <30s) and the provider matrix passes. @@ -25,6 +21,15 @@ Below that: the previous "Outstanding Items"/"Verification Evidence" entries tra ## Verification Evidence +This session (Issue #141 — Automatic Syncing, `claude/automatic-sync-141` stacked on `claude/mobile-source-control-density`): + +- `npx eslint .` — 0 errors, 0 warnings. +- `npm run build` (tsc + Obsidian 1.11.0 compat typecheck + esbuild) — passed. +- `npx vitest run` — 80 files / 1010 tests passed (baseline from the parent branch was 76 files / 958; +41 new Automatic Sync tests across settings, execution policy, service, guard, scheduler, startup, i18n, settings UI, and changelog, plus settings-literal updates). +- New coverage: `tests/logic/source-control/AutomaticSyncService.test.ts`, `tests/logic/source-control/SyncExecutionGuard.test.ts`, `tests/runtime/AutomaticSyncScheduler.test.ts`, `tests/ui/SettingsAutomaticSync.test.ts`; extended `tests/logic/sync/PushCoordinator.test.ts` (prompt vs skip), `tests/logic/source-control/SourceControlActionService.test.ts` (background mode, skipped-conflict OperationState edge case, manual-vs-background serialization), `tests/main.test.ts` (startup decision), `tests/runtime/createSyncRuntime.test.ts`, `tests/settings.test.ts`, `tests/i18n/index.test.ts`, `tests/changelog.test.ts`, `tests/ui/SettingsConnectionStatus.test.ts`. +- **Not verified in this environment:** manual Obsidian runtime verification (no executable Obsidian environment) and the real-provider E2E suite. The manual checklist is included in the #141 PR body. +- semantic-release owns the actual 1.7.0 version bump; `manifest.json`/`package.json`/`versions.json`/generated `CHANGELOG.md` were intentionally not hand-edited. + This session (explicit per-file sync actions, 7 commits on `claude/fix-source-control-explicit-sync-intent`): - Each commit individually verified before being made: `npx eslint .` (0 errors), `npx vitest run` (68 files, growing from 892 to 914 tests across the branch), `npm run build` (tsc + Obsidian 1.11.0 compat typecheck + esbuild) — all passed at every commit. diff --git a/session-handoff.md b/session-handoff.md index 48f0ad6..095d59a 100644 --- a/session-handoff.md +++ b/session-handoff.md @@ -1,25 +1,38 @@ # Session Handoff -**Date:** 2026-08-31 -**Active feature:** Issue #143 — reduce redundant real-provider E2E round trips. -**Working tree:** Complete and uncommitted; no production files changed. +**Date:** 2026-09-12 +**Active feature:** Issue #141 — Automatic Syncing (target v1.7.0). Implementation complete and locally verified; PR pending. +**Branch:** `claude/automatic-sync-141`, a child branch of `origin/claude/mobile-source-control-density` (`a1f01b3`, PR #156 still open). The #141 PR must be stacked with base `claude/mobile-source-control-density`; retarget the SAME PR to `main` only if #156 merges first. Do not commit #141 work onto #156. -## Completed +## Completed this session -1. Provider settings identity is now the selected `github` / `gitlab` / `gitea` provider in both SyncManager fixtures. -2. Added batch baseline helpers. The single-client multi-file scenarios (including mixed-100's 70 files) now create one baseline commit; two-client P0-2 does the same while mirroring verified metadata into client B. -3. Added fetch-once `GitVerifier.snapshot()` / `GitSnapshot`; `SourceControlScenario` invalidates and recreates a shared snapshot after remote mutations, and convergence assertions use one snapshot for all files/tree reads. -4. GitHub batch/rename/delete checks poll only branch-head movement, then verify one snapshot. GitLab and Gitea batch tests also assert exactly one new commit. -5. Added `e2e-tests/provider/suites/git-verifier.e2e.test.ts`, a local-git regression test proving all snapshot reads run after one fetch. -6. Registered that suite in `scripts/e2e-suites.txt`; the first CI run rejected it as unregistered before executing provider tests. +- Settings model: `automaticSyncEnabled` (false), `automaticSyncIntervalMinutes` (5), `automaticSyncOnStartup` (false); `normalizeAutomaticSyncIntervalMinutes` / `MIN_AUTOMATIC_SYNC_INTERVAL_MINUTES` in `src/settings/helpers.ts` reject zero/negative/NaN/non-numeric input so no rapid timer can be created. +- Settings UI (`src/ui/settings/GitLabSyncSettingTab.ts`): Automatic sync toggle, Sync interval, Sync on startup — all distinct from the existing Refresh status on startup. EN/zh-TW/zh-CN strings added. +- `SyncExecutionMode = 'interactive' | 'background'` on `SyncIntentExecutor.execute` / `SourceControlActionService.sync`; background auto-accepts the plan, opens no conflict UI, stays silent on success. +- `PushConflictBehavior = 'prompt' | 'skip'` at the `PushCoordinator.planSyncBatch` planning boundary (UI-free); `SyncWorkspace.planPush` / `SyncManager.planSyncBatch` pass it through. +- Skipped-conflict paths are excluded from commit/pull target sets and reset to idle, so they are never marked success (regression test in `SourceControlActionService.test.ts`). +- `SyncExecutionGuard`: manual work waits, automatic work try-acquires and skips. Owned by `SourceControlActionService`, injected into `SyncIntentExecutor`; immediate row actions + conflict resolution also serialized. +- `AutomaticSyncService` (`runOnce`): refresh → `ChangeRepository` → exclude synced/conflict → default intents via `ChangeActionPolicy` → background execute → refresh. Overlap-skipping and error-safe (never permanently locked). Wired in `createSyncRuntime`. +- `AutomaticSyncScheduler` (`src/runtime/AutomaticSyncScheduler.ts`): interval timer, idempotent `apply()`, `registerInterval`, `dispose()`. `main.ts` applies it on load and every `saveSettings`, disposes on unload. +- Startup: `handleLayoutReady()` runs background automatic sync (never opens Source Control) when enabled + on startup, else falls back to the legacy refresh-on-startup; no duplicate boot fetch. +- What's New: `src/changelog/1.7.0/index.ts` registered first in `src/changelog/index.ts` (EN/zh-TW/zh-CN headline, summary, 3 notable entries). ## Verification - `npx eslint .` — 0 errors, 0 warnings. -- `npx vitest run` — 68 files / 862 tests passed. -- `npx vitest -c vitest.e2e.config.ts run e2e-tests/provider/suites/git-verifier.e2e.test.ts` — 1 file / 1 test passed. -- `npm run build` — passed (including Obsidian 1.11.0 compatibility check). +- `npm run build` — passed (tsc + Obsidian 1.11.0 compat typecheck + esbuild). +- `npx vitest run` — 80 files / 1010 tests passed. +- NOT done: manual Obsidian runtime verification (no executable Obsidian here) and real-provider E2E. Both are explicitly noted in the PR body; do not claim they ran. -## Remaining +## Next steps -Run the provisioned real-provider matrix for GitHub, GitLab, and Gitea. Confirm mixed-100 stays within its 120s timeout and measure against the <60s / <30s targets before committing and pushing. +1. Commit and push `claude/automatic-sync-141`; open the stacked PR (base `claude/mobile-source-control-density`) titled `feat(sync): add automatic scheduled sync`, body includes `Closes #141` + the manual checklist. +2. If #156 merges: merge latest `main` into this SAME branch and retarget the SAME PR to `main`. +3. Manual Obsidian verification (desktop + mobile) against the checklist in the PR. +4. Do not hand-bump `manifest.json`/`package.json`/`versions.json`/generated `CHANGELOG.md`; semantic-release performs the 1.7.0 bump. + +## Gotchas + +- `tests/setup.ts` now has `ToggleComponent` and a faithful `.setting-item` `Setting` mock (name/desc/control children); `window.setInterval`/`clearInterval` delegate to globals so fake timers work. +- `tests/ui/SettingsAutomaticSync.test.ts` is added to the ESLint `display()`-deprecation exemption block (legacy pre-1.13 fallback path, same as the other two settings suites). +- All settings object literals in tests/e2e needed the three new fields; `tests/settings.test.ts` asserts the exact `DEFAULT_SETTINGS` shape. diff --git a/src/changelog/1.7.0/index.ts b/src/changelog/1.7.0/index.ts new file mode 100644 index 0000000..1492067 --- /dev/null +++ b/src/changelog/1.7.0/index.ts @@ -0,0 +1,43 @@ +import { type ChangelogRelease } from '../types'; + +export const release: ChangelogRelease = { + version: '1.7.0', + + headline: { + en: 'Automatic sync, on your schedule', + 'zh-tw': '依照你的時間自動同步', + 'zh-cn': '按照你的时间自动同步', + }, + summary: { + en: 'Automatically keep your vault and remote repository in sync on a configurable schedule, while leaving conflicts safely for manual resolution.', + 'zh-tw': '依照你設定的週期自動同步 Vault 與遠端儲存庫；遇到需要人工判斷的衝突時會安全略過，保留給你手動處理。', + 'zh-cn': '按照你设置的周期自动同步 Vault 与远程仓库；遇到需要人工判断的冲突时会安全跳过，保留给你手动处理。', + }, + + entries: [ + { + notable: true, + text: { + en: '⏱️ Automatic sync — Apply the default Sync action for pending changes on a configurable interval.', + 'zh-tw': '⏱️ 自動同步 — 依照可設定的週期，自動套用待處理變更的預設同步動作。', + 'zh-cn': '⏱️ 自动同步 — 按照可设置的周期，自动应用待处理更改的默认同步动作。', + }, + }, + { + notable: true, + text: { + en: '🚀 Optional sync on startup — Run one automatic sync right after Obsidian finishes loading.', + 'zh-tw': '🚀 可選的啟動時同步 — 在 Obsidian 載入完成後立即執行一次自動同步。', + 'zh-cn': '🚀 可选的启动时同步 — 在 Obsidian 加载完成后立即执行一次自动同步。', + }, + }, + { + notable: true, + text: { + en: '🛡️ Conflicts are skipped safely — Files that need manual resolution are left untouched while unrelated changes keep syncing.', + 'zh-tw': '🛡️ 衝突會安全略過 — 需要手動處理的檔案會保持原狀，其餘變更則照常同步。', + 'zh-cn': '🛡️ 冲突会安全跳过 — 需要手动处理的文件会保持原样，其余更改则照常同步。', + }, + }, + ], +}; diff --git a/src/changelog/index.ts b/src/changelog/index.ts index 0068044..d8648bb 100644 --- a/src/changelog/index.ts +++ b/src/changelog/index.ts @@ -1,6 +1,7 @@ import { compareVersions } from '../utils/version'; import { getActiveLocale } from '../i18n'; import { type ChangelogEntry, type ChangelogEntryText, type ChangelogRelease } from './types'; +import { release as release_1_7_0 } from './1.7.0'; import { release as release_1_6_0 } from './1.6.0'; import { release as release_1_5_0 } from './1.5.0'; import { release as release_1_4_0 } from './1.4.0'; @@ -30,6 +31,7 @@ export { * by exact string, so keep them in sync. */ export const CHANGELOG: ChangelogRelease[] = [ + release_1_7_0, release_1_6_0, release_1_5_0, release_1_4_0, diff --git a/src/i18n/locales/en.ts b/src/i18n/locales/en.ts index bac2a95..a2b32e6 100644 --- a/src/i18n/locales/en.ts +++ b/src/i18n/locales/en.ts @@ -30,6 +30,13 @@ const en = { 'settings.autoRefreshOnStartup.name': 'Refresh status on startup', 'settings.autoRefreshOnStartup.desc': 'Automatically refresh the Sync Status View when Obsidian finishes loading', + 'settings.automaticSync.name': 'Automatic sync', + 'settings.automaticSync.desc': 'Automatically apply the default Sync action for pending changes on a schedule, without opening the Source Control view', + 'settings.automaticSyncInterval.name': 'Sync interval (minutes)', + 'settings.automaticSyncInterval.desc': 'How often automatic sync runs. Minimum {min} minute.', + 'settings.automaticSyncOnStartup.name': 'Sync on startup', + 'settings.automaticSyncOnStartup.desc': 'Run one automatic sync after Obsidian finishes loading', + 'settings.ignorePatterns.name': 'Ignore patterns', 'settings.ignorePatterns.desc': 'Optional: .gitignore-style patterns (one per line) to exclude local files from sync, in addition to the repository\'s own .gitignore.', diff --git a/src/i18n/locales/zh-cn.ts b/src/i18n/locales/zh-cn.ts index 60898b6..15a2ba4 100644 --- a/src/i18n/locales/zh-cn.ts +++ b/src/i18n/locales/zh-cn.ts @@ -32,6 +32,13 @@ const zhCn: Partial> = { 'settings.autoRefreshOnStartup.name': '启动时刷新状态', 'settings.autoRefreshOnStartup.desc': 'Obsidian 加载完成后，自动刷新同步状态窗口', + 'settings.automaticSync.name': '自动同步', + 'settings.automaticSync.desc': '按照计划自动应用待处理更改的默认同步动作，不会打开源代码控制窗口', + 'settings.automaticSyncInterval.name': '同步间隔（分钟）', + 'settings.automaticSyncInterval.desc': '自动同步的执行频率。最短为 {min} 分钟。', + 'settings.automaticSyncOnStartup.name': '启动时同步', + 'settings.automaticSyncOnStartup.desc': 'Obsidian 加载完成后执行一次自动同步', + 'settings.ignorePatterns.name': '忽略规则', 'settings.ignorePatterns.desc': '选填：以 .gitignore 语法（每行一条）排除本机文件，会与远程仓库的 .gitignore 规则一并应用。', diff --git a/src/i18n/locales/zh-tw.ts b/src/i18n/locales/zh-tw.ts index dec9fc0..02a65f3 100644 --- a/src/i18n/locales/zh-tw.ts +++ b/src/i18n/locales/zh-tw.ts @@ -32,6 +32,13 @@ const zhTw: Partial> = { 'settings.autoRefreshOnStartup.name': '啟動時重新整理狀態', 'settings.autoRefreshOnStartup.desc': 'Obsidian 載入完成後，自動重新整理同步狀態視窗', + 'settings.automaticSync.name': '自動同步', + 'settings.automaticSync.desc': '依照排程自動套用待處理變更的預設同步動作，不會開啟原始碼控制視窗', + 'settings.automaticSyncInterval.name': '同步間隔（分鐘）', + 'settings.automaticSyncInterval.desc': '自動同步的執行頻率。最短為 {min} 分鐘。', + 'settings.automaticSyncOnStartup.name': '啟動時同步', + 'settings.automaticSyncOnStartup.desc': 'Obsidian 載入完成後執行一次自動同步', + 'settings.ignorePatterns.name': '忽略規則', 'settings.ignorePatterns.desc': '選填：以 .gitignore 語法（每行一條）排除本機檔案，會與遠端儲存庫的 .gitignore 規則一併套用。', diff --git a/src/logic/source-control/AutomaticSyncService.ts b/src/logic/source-control/AutomaticSyncService.ts new file mode 100644 index 0000000..0f0c4dd --- /dev/null +++ b/src/logic/source-control/AutomaticSyncService.ts @@ -0,0 +1,74 @@ +import type { SyncWorkspace } from '../sync/SyncWorkspace'; +import { defaultSyncAction } from './ChangeActionPolicy'; +import type { ChangeRepository } from './ChangeRepository'; +import type { SourceControlActionService } from './SourceControlActionService'; +import type { SyncIntentRequest } from './SyncIntent'; +import type { SyncChange } from './types'; + +export interface AutomaticSyncDependencies { + workspace: Pick; + changes: ChangeRepository; + actions: Pick; + /** Optional diagnostic sink; automatic runs stay silent on success. */ + onError?: (error: unknown) => void; +} + +/** + * Application-level use case for one automatic sync run. + * + * Reuses the existing Source Control application layer end to end: it does not + * classify changes, detect renames, route actions, plan push/pull, or talk to a + * provider itself. The sequence is refresh -> read ChangeRepository -> exclude + * synced/conflict -> build default intents via ChangeActionPolicy -> execute + * through the shared Sync Queue path in background mode -> refresh again. + * + * Timer mechanics deliberately live outside this service (plugin runtime + * scheduling); this class only knows how to run once. + */ +export class AutomaticSyncService { + private running = false; + + constructor(private readonly dependencies: AutomaticSyncDependencies) {} + + /** + * Runs at most one automatic sync at a time. A tick that fires while a run + * is already active is skipped rather than queued, and an execution error + * never leaves the service permanently locked. + */ + async runOnce(): Promise { + if (this.running) return; + this.running = true; + try { + await this.dependencies.workspace.refresh(); + const intents = this.pendingIntents(); + if (intents.length > 0) { + await this.dependencies.actions.sync(intents, 'background'); + // Re-read after execution so the refreshed status reflects the + // changes that were just applied (and keeps conflicts visible). + } + await this.dependencies.workspace.refresh(); + } catch (error) { + this.dependencies.onError?.(error); + } finally { + this.running = false; + } + } + + /** + * Every pending change except 'synced' (nothing to do) and 'conflict' + * (must be resolved manually). Actions come from the same default routing + * the manual Sync button uses, with no explicit override. + */ + private pendingIntents(): SyncIntentRequest[] { + return this.dependencies.changes + .getAll() + .filter(change => change.kind !== 'synced' && change.kind !== 'conflict') + .map(change => toDefaultIntent(change)); + } +} + +function toDefaultIntent(change: SyncChange): SyncIntentRequest { + // Route through the action policy so the default action has one source of + // truth; execution re-validates it against the live change kind anyway. + return { changeId: change.id, action: defaultSyncAction(change.kind) }; +} diff --git a/src/logic/source-control/SourceControlActionService.ts b/src/logic/source-control/SourceControlActionService.ts index 225d9ae..8513abf 100644 --- a/src/logic/source-control/SourceControlActionService.ts +++ b/src/logic/source-control/SourceControlActionService.ts @@ -5,7 +5,8 @@ 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 { SyncIntentExecutor, type SyncExecutionMode } from './SyncIntentExecutor'; +import { SyncExecutionGuard } from './SyncExecutionGuard'; import type { SyncIntentRequest } from './SyncIntent'; import type { ChangeId, SyncChange } from './types'; @@ -39,6 +40,12 @@ export interface SourceControlDiffContent { */ export class SourceControlActionService { private readonly syncIntentExecutor: SyncIntentExecutor; + /** + * Serializes every provider-mutating operation (the Sync Queue workflow and + * the immediate row actions) against automatic sync. User-triggered work + * waits for the lock; automatic work try-acquires and skips when busy. + */ + private readonly guard = new SyncExecutionGuard(); constructor( private readonly changes: ChangeRepository, @@ -52,9 +59,20 @@ export class SourceControlActionService { operations, workspace, syncResultNotifier, + this.guard, ); } + /** Runs `operation` while holding the shared execution guard (manual semantics: waits, never discards). */ + private async serialized(operation: () => Promise): Promise { + const release = await this.guard.acquire(); + try { + return await operation(); + } finally { + release(); + } + } + /** Adds one change to the Sync Queue. */ selectForSync(changeId: ChangeId): void { this.selection.selectForSync(changeId); @@ -103,13 +121,15 @@ export class SourceControlActionService { if (targets.length === 0) return; this.startAll(targets); - try { - const results = await this.workspace.push(targets.map(target => target.path)); - const failed = new Set(results.errors.map(error => error.file)); - this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); - } catch { - this.failAll(targets); - } + await this.serialized(async () => { + try { + const results = await this.workspace.push(targets.map(target => target.path)); + const failed = new Set(results.errors.map(error => error.file)); + this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); + } catch { + this.failAll(targets); + } + }); } /** Pulls one or more changes. */ @@ -118,13 +138,15 @@ export class SourceControlActionService { if (targets.length === 0) return; this.startAll(targets); - try { - const results = await this.workspace.pull(targets.map(target => target.path)); - const failed = new Set(results.errors.map(error => error.file)); - this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); - } catch { - this.failAll(targets); - } + await this.serialized(async () => { + try { + const results = await this.workspace.pull(targets.map(target => target.path)); + const failed = new Set(results.errors.map(error => error.file)); + this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); + } catch { + this.failAll(targets); + } + }); } /** Deletes one or more changes from the remote only. */ @@ -133,36 +155,41 @@ export class SourceControlActionService { if (targets.length === 0) return; this.startAll(targets); - try { - const result = await this.workspace.deleteRemote(targets.map(target => target.path)); - const failed = new Set(result.errors.map(error => error.path)); - this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); - } catch { - this.failAll(targets); - } + await this.serialized(async () => { + try { + const result = await this.workspace.deleteRemote(targets.map(target => target.path)); + const failed = new Set(result.errors.map(error => error.path)); + this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); + } catch { + this.failAll(targets); + } + }); } /** * Executes the whole Sync Queue as one explicit-intent workflow. * Kept as the stable UI-facing facade; orchestration lives in - * SyncIntentExecutor. + * SyncIntentExecutor. `mode` is per-execution: manual Sync stays + * interactive, Automatic Sync passes `background`. */ - async sync(intents: readonly SyncIntentRequest[]): Promise { - await this.syncIntentExecutor.execute(intents); + async sync(intents: readonly SyncIntentRequest[], mode: SyncExecutionMode = 'interactive'): Promise { + await this.syncIntentExecutor.execute(intents, mode); } /** Deletes one or more changes from the local vault only. */ async deleteLocal(changeIds: readonly ChangeId[]): Promise { const targets = this.resolve(changeIds); - for (const target of targets) { - this.operations.start(target.id); - try { - await this.workspace.deleteLocal(target.path); - this.operations.succeed(target.id); - } catch { - this.operations.fail(target.id); + await this.serialized(async () => { + for (const target of targets) { + this.operations.start(target.id); + try { + await this.workspace.deleteLocal(target.path); + this.operations.succeed(target.id); + } catch { + this.operations.fail(target.id); + } } - } + }); } /** @@ -174,6 +201,11 @@ export class SourceControlActionService { const change = this.changes.getById(changeId); if (!change) return; + await this.serialized(() => this.resolveConflictLocked(change, resolution)); + } + + private async resolveConflictLocked(change: SyncChange, resolution: ConflictResolution): Promise { + const changeId = change.id; this.operations.start(changeId); try { if (resolution === 'local') { diff --git a/src/logic/source-control/SyncExecutionGuard.ts b/src/logic/source-control/SyncExecutionGuard.ts new file mode 100644 index 0000000..c2ede60 --- /dev/null +++ b/src/logic/source-control/SyncExecutionGuard.ts @@ -0,0 +1,53 @@ +/** + * Application-level serialization for sync mutations. + * + * Manual work acquires (and waits for) the guard so a user-triggered sync is + * never discarded. Automatic work uses {@link tryAcquire} and skips its tick + * when the sync execution path is already busy, rather than queueing a backlog + * or running provider mutations concurrently. + * + * Deliberately minimal: one lock, no queue priority, no cancellation/preemption. + */ +export class SyncExecutionGuard { + private locked = false; + private readonly waiters: Array<() => void> = []; + + get isLocked(): boolean { + return this.locked; + } + + /** Waits for the guard, then returns a one-shot release function. */ + async acquire(): Promise<() => void> { + if (!this.locked) { + this.locked = true; + return this.releaseOnce(); + } + await new Promise(resolve => this.waiters.push(resolve)); + return this.releaseOnce(); + } + + /** Acquires only if idle; returns null (and runs nothing) when already busy. */ + tryAcquire(): (() => void) | null { + if (this.locked) return null; + this.locked = true; + return this.releaseOnce(); + } + + /** + * Hands the lock directly to the next waiter on release so a waiting + * manual operation doesn't race a fresh automatic tick for it. + */ + private releaseOnce(): () => void { + let released = false; + return () => { + if (released) return; + released = true; + const next = this.waiters.shift(); + if (next) { + next(); + } else { + this.locked = false; + } + }; + } +} diff --git a/src/logic/source-control/SyncIntentExecutor.ts b/src/logic/source-control/SyncIntentExecutor.ts index 9db08ec..77dc9f2 100644 --- a/src/logic/source-control/SyncIntentExecutor.ts +++ b/src/logic/source-control/SyncIntentExecutor.ts @@ -12,6 +12,7 @@ import type { ChangeRepository } from './ChangeRepository'; import type { OperationState } from './OperationState'; import type { SyncExecutionResult, SyncResultNotificationPort } from './SyncResultNotifier'; import type { SyncIntentRequest } from './SyncIntent'; +import { SyncExecutionGuard } from './SyncExecutionGuard'; import type { SyncChange } from './types'; interface ResolvedSyncIntent { @@ -30,6 +31,22 @@ interface ConfirmedSyncPlan { confirmed: boolean; } +/** + * Per-execution interaction policy for the shared Sync Queue path. + * + * - `interactive` (manual Sync): unchanged behavior -- one merged plan, one + * confirmation modal, batch conflict interaction, and a result notice. + * - `background` (automatic sync): build/validate the same plan but + * auto-accept it, never open a conflict modal, skip conflicting paths + * instead of aborting, and stay silent on success. Errors are still logged + * by the caller. + * + * This is deliberately an explicit mode rather than a set of independent + * booleans, so invalid combinations (e.g. confirm + showConflictModal) can't + * be expressed. + */ +export type SyncExecutionMode = 'interactive' | 'background'; + /** * Executes the Sync Queue use-case from explicit user intent. * @@ -50,9 +67,24 @@ export class SyncIntentExecutor { private readonly operations: OperationState, private readonly workspace: SyncWorkspace, private readonly notifier: SyncResultNotificationPort = { notify: () => {} }, + private readonly guard: SyncExecutionGuard = new SyncExecutionGuard(), ) {} - async execute(intents: readonly SyncIntentRequest[]): Promise { + async execute(intents: readonly SyncIntentRequest[], mode: SyncExecutionMode = 'interactive'): Promise { + // Serialize automatic mutations against user-triggered ones. Automatic + // work skips its tick entirely when the path is busy; manual work waits + // so a user action is never discarded. + const release = mode === 'background' ? this.guard.tryAcquire() : await this.guard.acquire(); + if (!release) return; + + try { + await this.executeLocked(intents, mode); + } finally { + release(); + } + } + + private async executeLocked(intents: readonly SyncIntentRequest[], mode: SyncExecutionMode): Promise { const resolved = this.resolveIntents(intents); if (resolved.length === 0) return; @@ -61,26 +93,38 @@ export class SyncIntentExecutor { let plan: ConfirmedSyncPlan | null; try { - plan = await this.planAndConfirm(buckets); + plan = await this.planAndConfirm(buckets, mode); } catch { this.failAll(targets); - this.notifier.notify({ ...emptyExecutionResult(), failed: targets.length }); + if (mode !== 'background') this.notifier.notify({ ...emptyExecutionResult(), failed: targets.length }); return; } if (!plan || !plan.confirmed) return; + // Paths the planner left out because they still need manual conflict + // resolution. They must never be reported as successfully synced or + // transitioned to operation success merely because they were in the + // original target set -- after the final refresh they remain conflicts. + const skippedPaths = new Set(plan.plannedPush.conflictedPaths); + this.startAll(targets); const summary = emptyExecutionResult(); if (hasRemoteMutations(plan.plannedPush, buckets.deleteRemote)) { - await this.commitRemoteBucket(plan.plannedPush, buckets.push, buckets.deleteRemote, summary); + await this.commitRemoteBucket(plan.plannedPush, buckets.push, buckets.deleteRemote, summary, skippedPaths); } if (buckets.pull.length > 0) { - await this.applyPullBucket(buckets.pull, summary); + await this.applyPullBucket(buckets.pull, summary, skippedPaths); + } + + // Any skipped/conflicted target that a commit or pull may have marked + // (or left running) is reset to idle rather than success/failure. + for (const target of targets) { + if (skippedPaths.has(target.path)) this.operations.reset(target.id); } - this.notifier.notify(summary); + if (mode !== 'background') this.notifier.notify(summary); } private resolveIntents(intents: readonly SyncIntentRequest[]): ResolvedSyncIntent[] { @@ -106,13 +150,20 @@ export class SyncIntentExecutor { return buckets; } - private async planAndConfirm(buckets: SyncIntentBuckets): Promise { + private async planAndConfirm( + buckets: SyncIntentBuckets, + mode: SyncExecutionMode, + ): Promise { + // The execution policy is per-run: interactive planning may prompt and + // cancel the batch on conflict, background planning skips conflicts and + // continues with the safe paths. Both validate the normal sync plan. + const conflictBehavior = mode === 'background' ? 'skip' : 'prompt'; const plannedPush = buckets.push.length > 0 - ? await this.workspace.planPush(buckets.push.map(change => change.path)) + ? await this.workspace.planPush(buckets.push.map(change => change.path), conflictBehavior) : emptyPlannedBatch(); - // Batch conflict resolution is an interactive planning step. If the - // user cancels it, no merged review modal or mutation should follow. + // An interactive batch may be aborted by cancelling conflict + // resolution; background planning never returns `cancelled`. if (plannedPush.cancelled) return null; const pullPlan = buckets.pull.length > 0 @@ -135,9 +186,11 @@ export class SyncIntentExecutor { if (isSyncPlanEmpty(mergedPlan)) return null; + // Background mode auto-accepts the reviewed plan; it never opens the + // final confirmation modal. return { plannedPush, - confirmed: await this.workspace.confirmPlan(mergedPlan, 'sync'), + confirmed: mode === 'background' ? true : await this.workspace.confirmPlan(mergedPlan, 'sync'), }; } @@ -146,8 +199,9 @@ export class SyncIntentExecutor { pushTargets: readonly SyncChange[], deleteTargets: readonly SyncChange[], summary: SyncExecutionResult, + skippedPaths: ReadonlySet, ): Promise { - const targets = [...pushTargets, ...deleteTargets]; + const targets = [...pushTargets, ...deleteTargets].filter(target => !skippedPaths.has(target.path)); try { const deleteEntries: DeleteQueueEntry[] = deleteTargets.map(change => ({ path: change.path, @@ -187,20 +241,23 @@ export class SyncIntentExecutor { private async applyPullBucket( pullTargets: readonly SyncChange[], summary: SyncExecutionResult, + skippedPaths: ReadonlySet, ): Promise { + const targets = pullTargets.filter(target => !skippedPaths.has(target.path)); try { + if (targets.length === 0) return; const results = await this.workspace.applyPull( - pullTargets.map(change => change.path), + targets.map(change => change.path), { notify: false }, ); const failed = new Set(results.errors.map(error => error.file)); - this.finishAll(pullTargets, path => failed.has(path) ? 'failed' : 'success'); + this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); summary.downloaded += results.added + results.updated; summary.failed += results.failed; summary.conflicts += results.conflicts; summary.errors.push(...results.errors); } catch { - this.failAll(pullTargets); + this.failAll(targets); summary.failed += pullTargets.length; } } diff --git a/src/logic/sync/PushCoordinator.ts b/src/logic/sync/PushCoordinator.ts index 0c6ce95..370a7db 100644 --- a/src/logic/sync/PushCoordinator.ts +++ b/src/logic/sync/PushCoordinator.ts @@ -23,6 +23,19 @@ import { type BatchOutcome = 'done' | 'unchanged' | 'conflict'; +/** + * How push planning reacts when a candidate conflicts with the remote. + * + * - `prompt` (interactive/manual): ask the user to resolve the batch through + * the interaction port; cancelling aborts the whole batch. + * - `skip` (background/automatic): do not touch UI, leave each conflicting + * path out of the plan, and continue with the safe paths. + * + * Deliberately UI-independent: `PushCoordinator` never learns about ExecutionMode + * or Obsidian, only this small behavior switch supplied by the caller. + */ +export type PushConflictBehavior = 'prompt' | 'skip'; + interface BatchPushPlan { pushes: PushQueueEntry[]; moves: MoveQueueEntry[]; @@ -123,6 +136,7 @@ export class PushCoordinator { files: Array, onProgress?: (current: number, total: number, fileName: string) => void, remoteTree?: GitTreeEntry[], + conflictBehavior: PushConflictBehavior = 'prompt', ): Promise { const syncableFiles = files.filter(file => file && !this.dependencies.isPathIgnored(this.fileInfo(file).path)); if (syncableFiles.length === 0) { @@ -150,6 +164,26 @@ export class PushCoordinator { errors: immediate.errors, syncedPaths: immediate.syncedPaths, }; + + // Background planning skips the interaction port entirely: every + // conflicting path is left out of the plan (safe paths continue), and + // the batch is never cancelled. Interactive planning keeps the existing + // prompt-and-maybe-cancel behavior untouched. + if (conflictBehavior === 'skip') { + const skippedConflicts = plan.conflicts.length + plan.autoSkipped.length; + return { + reviewPlan: this.buildReviewPlan(plan, plan.conflicts, []), + pushes: plan.pushes, + moves: plan.moves, + keepRemote: [], + keepLocal: [], + skippedConflicts, + conflictedPaths: this.conflictedPaths(plan), + cancelled: false, + immediate, + }; + } + const skipped: BatchPushConflict[] = []; const keepRemote: BatchPushConflict[] = []; const keepLocal: BatchPushConflict[] = []; diff --git a/src/logic/sync/SyncManager.ts b/src/logic/sync/SyncManager.ts index 478e22f..31330f9 100644 --- a/src/logic/sync/SyncManager.ts +++ b/src/logic/sync/SyncManager.ts @@ -18,7 +18,7 @@ import { SyncScanner } from './SyncScanner'; import { ConflictResolver } from './ConflictResolver'; import { SyncExecutor } from './SyncExecutor'; import { PullCoordinator } from './PullCoordinator'; -import { PushCoordinator } from './PushCoordinator'; +import { PushCoordinator, type PushConflictBehavior } from './PushCoordinator'; import { HeadlessSyncInteraction, type ConflictDiffLoader, @@ -315,8 +315,9 @@ export class SyncManager { files: (TFile | string)[], onProgress?: (current: number, total: number, fileName: string) => void, remoteTree?: GitTreeEntry[], + conflictBehavior?: PushConflictBehavior, ): ReturnType { - return this.pushCoordinator.planSyncBatch(files, onProgress, remoteTree); + return this.pushCoordinator.planSyncBatch(files, onProgress, remoteTree, conflictBehavior); } /** Commits already-planned pushes/moves/deletions as one provider mutation set. */ diff --git a/src/logic/sync/SyncWorkspace.ts b/src/logic/sync/SyncWorkspace.ts index c1c0107..1c548ef 100644 --- a/src/logic/sync/SyncWorkspace.ts +++ b/src/logic/sync/SyncWorkspace.ts @@ -3,7 +3,7 @@ import type { GitServiceInterface, GitTreeEntry } from '../../services/git-servi import { getServiceName, type GitLabFilesPushSettings } from '../../settings'; import type { GitignoreManager } from '../gitignore-manager'; import type { FileStatus, SyncStatusService } from '../sync-status-service'; -import type { PlannedPushBatch } from './PushCoordinator'; +import type { PlannedPushBatch, PushConflictBehavior } from './PushCoordinator'; import { RemoteDeleteExecutor, type RemoteDeleteResult } from './RemoteDeleteExecutor'; import { SyncDiffService } from './SyncDiffService'; import type { SyncManager } from './SyncManager'; @@ -45,8 +45,13 @@ export interface SyncWorkspace { getDiff(path: string): Promise; /** Repo-relative path a provider mutation needs for a given vault path. */ toRepoPath(path: string): string; - /** Classifies and conflict-resolves a push batch without confirming or committing — for a unified Sync Plan. */ - planPush(paths: readonly string[]): Promise; + /** + * Classifies and conflict-resolves a push batch without confirming or + * committing — for a unified Sync Plan. `conflictBehavior` selects + * interactive prompt-and-maybe-cancel ('prompt', default) or + * skip-conflicts-and-continue ('skip', for background execution). + */ + planPush(paths: readonly string[], conflictBehavior?: PushConflictBehavior): Promise; /** Computes what a pull batch would do, without writing anything — for a unified Sync Plan. */ planPull(paths: readonly string[]): Promise; /** Applies an already-confirmed pull batch without showing its own confirm modal. */ @@ -183,9 +188,9 @@ export class SyncManagerWorkspace implements SyncWorkspace { return this.dependencies.normalizePath(path); } - async planPush(paths: readonly string[]): Promise { + async planPush(paths: readonly string[], conflictBehavior: PushConflictBehavior = 'prompt'): Promise { const remoteTree = await this.reusableRemoteTree(); - return this.dependencies.manager().planSyncBatch([...paths], undefined, remoteTree); + return this.dependencies.manager().planSyncBatch([...paths], undefined, remoteTree, conflictBehavior); } async planPull(paths: readonly string[]): Promise { @@ -254,7 +259,7 @@ export class BoundarySyncWorkspace implements SyncWorkspace { trackRename(newPath: string, oldPath: string): Promise { return this.getManager().trackRename(newPath, oldPath); } getDiff(path: string): Promise { return this.boundaries.getDiff(path); } toRepoPath(path: string): string { return path; } - planPush(paths: readonly string[]): Promise { return this.getManager().planSyncBatch([...paths]); } + planPush(paths: readonly string[], conflictBehavior?: PushConflictBehavior): Promise { return this.getManager().planSyncBatch([...paths], undefined, undefined, conflictBehavior); } planPull(paths: readonly string[]): Promise { return this.getManager().planPullBatch([...paths]); } applyPull(paths: readonly string[], options?: PullExecutionOptions): Promise { return this.getManager().applyPullBatch([...paths], undefined, undefined, options); } commitResolvedBatch( diff --git a/src/main.ts b/src/main.ts index 2a5c6d4..2625a03 100644 --- a/src/main.ts +++ b/src/main.ts @@ -24,6 +24,8 @@ import type { SyncSelectionStore } from './logic/source-control/SyncSelectionSto import type { SourceControlViewModel } from './logic/source-control/SourceControlViewModel'; import type { SourceControlActionService } from './logic/source-control/SourceControlActionService'; import { createSyncRuntime } from './runtime/createSyncRuntime'; +import { AutomaticSyncScheduler } from './runtime/AutomaticSyncScheduler'; +import type { AutomaticSyncService } from './logic/source-control/AutomaticSyncService'; import { filterFilesByVaultFolder as scopeFilterFiles, filterPathByVaultFolder as scopeFilterPath, @@ -52,6 +54,8 @@ export default class GitLabFilesPush extends Plugin { refreshState: RefreshState; sourceControlViewModel: SourceControlViewModel; sourceControlActions: SourceControlActionService; + automaticSync: AutomaticSyncService; + private automaticSyncScheduler?: AutomaticSyncScheduler; private disposeSyncRuntime?: () => void; private gitignoreConfigKey = ''; private pushRibbonEl: HTMLElement; @@ -114,7 +118,14 @@ export default class GitLabFilesPush extends Plugin { this.refreshState = runtime.refreshState; this.sourceControlViewModel = runtime.sourceControlViewModel; this.sourceControlActions = runtime.sourceControlActions; + this.automaticSync = runtime.automaticSync; this.disposeSyncRuntime = () => runtime.dispose(); + this.automaticSyncScheduler = new AutomaticSyncScheduler({ + getSettings: () => this.settings, + run: () => this.automaticSync.runOnce(), + registerInterval: id => this.registerInterval(id), + }); + this.automaticSyncScheduler.apply(); this.statusBarEl = this.addStatusBarItem(); this.statusBarEl.addClass('gfs-status-bar-connection'); @@ -276,18 +287,29 @@ export default class GitLabFilesPush extends Plugin { }) ); - this.app.workspace.onLayoutReady(() => { - // Legacy workspaces may hold more than one persisted sync-status - // leaf (duplicates accumulated across old plugin versions). - // Normalize first so startup activation reuses a single canonical - // leaf instead of revealing one duplicate while others linger. - this.normalizeSourceControlLeaves(); - if (this.settings.autoRefreshOnStartup) void this.refreshSyncStatusOnStartup(); - }); + this.app.workspace.onLayoutReady(() => this.handleLayoutReady()); await this.checkForUpdateNotice(); } + /** + * Startup decision after Obsidian's layout is ready: + * - legacy workspaces may hold duplicate persisted leaves; normalize first + * so activation reuses one canonical leaf; + * - startup Automatic Sync runs its own authoritative refresh in the + * background and must NOT open/focus Source Control, and it supersedes the + * legacy refresh-on-startup so boot doesn't fetch twice; + * - otherwise the existing refresh-on-startup behavior is preserved. + */ + private handleLayoutReady(): void { + this.normalizeSourceControlLeaves(); + if (this.settings.automaticSyncEnabled && this.settings.automaticSyncOnStartup) { + void this.automaticSync.runOnce(); + } else if (this.settings.autoRefreshOnStartup) { + void this.refreshSyncStatusOnStartup(); + } + } + private async refreshSyncStatusOnStartup(): Promise { await this.activateSourceControlView(); await this.sourceControlViewModel.refresh('startup'); @@ -678,7 +700,11 @@ export default class GitLabFilesPush extends Plugin { // Cleanup of registered components (views, commands, DOM/vault event // listeners) is handled by Obsidian. The sync runtime's cross-object // wiring (the ChangeRepository subscription) isn't Obsidian-managed, - // so it's disposed explicitly. + // so it's disposed explicitly. The automatic-sync timer is also stopped + // here even though registerInterval covers unload, so an interval is + // cleared the moment the plugin is disabled rather than lingering. + this.automaticSyncScheduler?.dispose(); + this.automaticSyncScheduler = undefined; this.disposeSyncRuntime?.(); this.disposeSyncRuntime = undefined; } @@ -693,5 +719,8 @@ export default class GitLabFilesPush extends Plugin { this.initializeGitService(); this.updateGitignoreManager(); this.updateRibbonTooltip(); + // Interval/enabled changes must take effect without a plugin reload; + // unchanged values are a no-op (see AutomaticSyncScheduler.apply). + this.automaticSyncScheduler?.apply(); } } diff --git a/src/runtime/AutomaticSyncScheduler.ts b/src/runtime/AutomaticSyncScheduler.ts new file mode 100644 index 0000000..f6721cd --- /dev/null +++ b/src/runtime/AutomaticSyncScheduler.ts @@ -0,0 +1,68 @@ +import { DEFAULT_SETTINGS } from '../settings/model'; +import { normalizeAutomaticSyncIntervalMinutes } from '../settings/helpers'; + +export interface AutomaticSyncScheduleSettings { + automaticSyncEnabled: boolean; + automaticSyncIntervalMinutes: number; +} + +export interface AutomaticSyncSchedulerDependencies { + /** Reads the current setting each time the schedule is (re)applied. */ + getSettings(): AutomaticSyncScheduleSettings; + /** Runs one background automatic sync. Overlapping ticks are skipped by the service. */ + run(): Promise; + /** Obsidian's `registerInterval`, so the plugin unload clears the timer too. */ + registerInterval(id: number): number; +} + +/** + * Owns only the timer for Automatic Sync. Whether a run should happen, and what + * it does, stays in AutomaticSyncService; Obsidian lifecycle stays in main.ts. + * + * Re-applying the schedule is idempotent for an unchanged enabled/interval + * pair, so frequent settings saves (e.g. token typing) don't postpone the next + * tick. A genuine enabled/interval change clears the old timer immediately and + * installs the new one, so it takes effect without a plugin reload. + */ +export class AutomaticSyncScheduler { + private timer: number | null = null; + private appliedKey = ''; + + constructor(private readonly dependencies: AutomaticSyncSchedulerDependencies) {} + + get isScheduled(): boolean { + return this.timer !== null; + } + + /** Clears and installs the timer to match the current settings. */ + apply(): void { + const settings = this.dependencies.getSettings(); + const minutes = normalizeAutomaticSyncIntervalMinutes( + settings.automaticSyncIntervalMinutes, + DEFAULT_SETTINGS.automaticSyncIntervalMinutes, + ); + const key = `${settings.automaticSyncEnabled ? 'on' : 'off'}:${minutes}`; + if (key === this.appliedKey) return; + this.appliedKey = key; + + this.clearTimer(); + if (!settings.automaticSyncEnabled) return; + + this.timer = window.setInterval(() => { + void this.dependencies.run(); + }, minutes * 60_000); + this.dependencies.registerInterval(this.timer); + } + + /** Stops any scheduled execution. */ + dispose(): void { + this.clearTimer(); + this.appliedKey = ''; + } + + private clearTimer(): void { + if (this.timer === null) return; + window.clearInterval(this.timer); + this.timer = null; + } +} diff --git a/src/runtime/createSyncRuntime.ts b/src/runtime/createSyncRuntime.ts index f696592..122cd43 100644 --- a/src/runtime/createSyncRuntime.ts +++ b/src/runtime/createSyncRuntime.ts @@ -12,9 +12,11 @@ import { RefreshState } from '../logic/source-control/RefreshState'; import { SyncSelectionStore } from '../logic/source-control/SyncSelectionStore'; import { SourceControlViewModel } from '../logic/source-control/SourceControlViewModel'; import { SourceControlActionService } from '../logic/source-control/SourceControlActionService'; +import { AutomaticSyncService } from '../logic/source-control/AutomaticSyncService'; import { SyncResultNotifier } from '../logic/source-control/SyncResultNotifier'; import { toSyncChanges } from '../logic/source-control/FileStatusAdapter'; import { ObsidianSyncInteraction } from '../ui/ObsidianSyncInteraction'; +import { logger } from '../utils/logger'; export interface SyncRuntimeDependencies { app: App; @@ -45,6 +47,7 @@ export interface SyncRuntime { refreshState: RefreshState; sourceControlViewModel: SourceControlViewModel; sourceControlActions: SourceControlActionService; + automaticSync: AutomaticSyncService; /** Tears down cross-object wiring (the ChangeRepository subscription) that Obsidian does not manage. */ dispose(): void; } @@ -115,6 +118,12 @@ export function createSyncRuntime(deps: SyncRuntimeDependencies): SyncRuntime { syncWorkspace, new SyncResultNotifier(deps.notify), ); + const automaticSync = new AutomaticSyncService({ + workspace: syncWorkspace, + changes: changeRepository, + actions: sourceControlActions, + onError: error => logger.error('Automatic sync failed', error), + }); // Selection-intent reconciliation is wired here, at the composition // root, rather than inside SourceControlViewModel: it is a write-side @@ -144,6 +153,7 @@ export function createSyncRuntime(deps: SyncRuntimeDependencies): SyncRuntime { refreshState, sourceControlViewModel, sourceControlActions, + automaticSync, dispose: () => { unsubscribeChangeRepository(); unsubscribeSelectionReconciliation(); diff --git a/src/settings/helpers.ts b/src/settings/helpers.ts index fded38c..9058663 100644 --- a/src/settings/helpers.ts +++ b/src/settings/helpers.ts @@ -1,5 +1,28 @@ import type { GitLabFilesPushSettings, SymlinkHandling, SyncMetadata } from './model'; +/** + * Smallest interval an automatic sync may be scheduled for, in minutes. The + * settings UI also clamps to this, but the scheduler resolves through this + * helper as well so an invalid persisted value (0, negative, NaN, a stray + * string from older data) can never produce a tight/zero-interval timer. + */ +export const MIN_AUTOMATIC_SYNC_INTERVAL_MINUTES = 1; + +/** + * Coerces a stored/typed automatic-sync interval to a valid minute count. + * Non-finite, non-numeric, or below the minimum values fall back to the + * default, so older settings that predate this field (or hand-edited data) + * can never create a rapid/zero-delay timer. + */ +export function normalizeAutomaticSyncIntervalMinutes( + value: unknown, + fallback: number, +): number { + const parsed = typeof value === 'number' ? value : Number(value); + if (!Number.isFinite(parsed) || parsed < MIN_AUTOMATIC_SYNC_INTERVAL_MINUTES) return fallback; + return Math.floor(parsed); +} + /** * Metadata written before `lastKnownPath` was introduced used its record key * as the path. Keep that format eligible for rename reconciliation. diff --git a/src/settings/model.ts b/src/settings/model.ts index ca91cfe..b71dab9 100644 --- a/src/settings/model.ts +++ b/src/settings/model.ts @@ -51,6 +51,12 @@ export interface GitLabFilesPushSettings { language: LanguageSetting; /** Refresh the sync status automatically after Obsidian finishes loading. */ autoRefreshOnStartup: boolean; + /** Run the default Sync action for pending changes automatically on a schedule. */ + automaticSyncEnabled: boolean; + /** How often scheduled automatic sync runs, in minutes. Minimum enforced by `normalizeAutomaticSyncIntervalMinutes`. */ + automaticSyncIntervalMinutes: number; + /** Run one background automatic sync once Obsidian finishes loading, in addition to the interval. */ + automaticSyncOnStartup: boolean; } export const DEFAULT_SETTINGS: GitLabFilesPushSettings = { @@ -75,4 +81,7 @@ export const DEFAULT_SETTINGS: GitLabFilesPushSettings = { bannerDismissedVersion: '', language: 'system', autoRefreshOnStartup: true, + automaticSyncEnabled: false, + automaticSyncIntervalMinutes: 5, + automaticSyncOnStartup: false, }; diff --git a/src/ui/settings/GitLabSyncSettingTab.ts b/src/ui/settings/GitLabSyncSettingTab.ts index 462a5ba..af673dd 100644 --- a/src/ui/settings/GitLabSyncSettingTab.ts +++ b/src/ui/settings/GitLabSyncSettingTab.ts @@ -11,8 +11,8 @@ 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'; +import { DEFAULT_SETTINGS, type GitLabFilesPushSettings, type GitServiceType, type SymlinkHandling } from '../../settings/model'; +import { getServiceName, getEffectiveSymlinkHandling, normalizeAutomaticSyncIntervalMinutes, MIN_AUTOMATIC_SYNC_INTERVAL_MINUTES } from '../../settings/helpers'; // Minimal shape of Obsidian >= 1.13's SettingDefinitionItem. Declared locally so // the plugin still type-checks against older Obsidian typings (minAppVersion @@ -279,6 +279,43 @@ export class GitLabSyncSettingTab extends PluginSettingTab { FolderSuggest.attach(this.app, text.inputEl); }); + new Setting(containerEl) + .setName(t('settings.automaticSync.name')) + .setDesc(t('settings.automaticSync.desc')) + .addToggle(toggle => toggle + .setValue(this.host.settings.automaticSyncEnabled) + .onChange((value) => { + this.host.settings.automaticSyncEnabled = value; + void this.host.saveSettings(); + this.refresh(); + })); + + new Setting(containerEl) + .setName(t('settings.automaticSyncInterval.name')) + .setDesc(t('settings.automaticSyncInterval.desc', { min: MIN_AUTOMATIC_SYNC_INTERVAL_MINUTES })) + .setDisabled(!this.host.settings.automaticSyncEnabled) + .addText(text => text + .setPlaceholder(String(DEFAULT_SETTINGS.automaticSyncIntervalMinutes)) + .setValue(String(this.host.settings.automaticSyncIntervalMinutes)) + .onChange((value) => { + this.host.settings.automaticSyncIntervalMinutes = normalizeAutomaticSyncIntervalMinutes( + value.trim() === '' ? Number.NaN : value, + DEFAULT_SETTINGS.automaticSyncIntervalMinutes, + ); + void this.host.saveSettings(); + })); + + new Setting(containerEl) + .setName(t('settings.automaticSyncOnStartup.name')) + .setDesc(t('settings.automaticSyncOnStartup.desc')) + .setDisabled(!this.host.settings.automaticSyncEnabled) + .addToggle(toggle => toggle + .setValue(this.host.settings.automaticSyncOnStartup) + .onChange((value) => { + this.host.settings.automaticSyncOnStartup = value; + void this.host.saveSettings(); + })); + new Setting(containerEl) .setName(t('settings.autoRefreshOnStartup.name')) .setDesc(t('settings.autoRefreshOnStartup.desc')) diff --git a/tests/changelog.test.ts b/tests/changelog.test.ts index 6fdc47a..3dec989 100644 --- a/tests/changelog.test.ts +++ b/tests/changelog.test.ts @@ -46,7 +46,45 @@ describe('1.5.0 release notes', () => { }); }); +describe('1.7.0 release notes', () => { + it('is registered in the changelog', () => { + expect(CHANGELOG.some(release => release.version === '1.7.0')).toBe(true); + }); + + it('provides a headline and summary in every supported language', () => { + const release = CHANGELOG.find(r => r.version === '1.7.0'); + expect(release?.headline?.en).toBeTruthy(); + expect(release?.headline?.['zh-tw']).toBeTruthy(); + expect(release?.headline?.['zh-cn']).toBeTruthy(); + expect(release?.summary?.en).toBeTruthy(); + expect(release?.summary?.['zh-tw']).toBeTruthy(); + expect(release?.summary?.['zh-cn']).toBeTruthy(); + }); + + it('has notable entries covering scheduled sync, startup sync, and safe conflict skipping', () => { + const release = CHANGELOG.find(r => r.version === '1.7.0'); + const notable = release?.entries.filter(entry => entry.notable) ?? []; + expect(notable.length).toBeGreaterThanOrEqual(3); + for (const entry of notable) { + expect(entry.text.en).toBeTruthy(); + expect(entry.text['zh-tw']).toBeTruthy(); + expect(entry.text['zh-cn']).toBeTruthy(); + } + }); + + it('is surfaced as an unseen release for a vault upgrading from 1.6.0', () => { + const unseen = getUnseenReleases(CHANGELOG, '1.6.0'); + expect(unseen.map(release => release.version)).toContain('1.7.0'); + }); +}); + describe('CHANGELOG ordering and content', () => { + it('lists 1.7.0 before 1.6.0 before 1.5.0', () => { + const versions = CHANGELOG.map(r => r.version); + expect(versions.indexOf('1.7.0')).toBeLessThan(versions.indexOf('1.6.0')); + expect(versions.indexOf('1.7.0')).toBeGreaterThanOrEqual(0); + }); + it('lists 1.6.0 before 1.5.0', () => { const versions = CHANGELOG.map(r => r.version); expect(versions.indexOf('1.6.0')).toBeLessThan(versions.indexOf('1.5.0')); diff --git a/tests/i18n/index.test.ts b/tests/i18n/index.test.ts index 9030bbc..f3fee87 100644 --- a/tests/i18n/index.test.ts +++ b/tests/i18n/index.test.ts @@ -52,6 +52,49 @@ describe('i18n', () => { expect(typeof t('confirmModal.title')).toBe('string'); }); + describe('automatic sync settings keys', () => { + const keys = [ + 'settings.automaticSync.name', + 'settings.automaticSync.desc', + 'settings.automaticSyncInterval.name', + 'settings.automaticSyncInterval.desc', + 'settings.automaticSyncOnStartup.name', + 'settings.automaticSyncOnStartup.desc', + ] as const; + + it('exist and resolve in EN', () => { + setMomentLocale(undefined); + for (const key of keys) { + expect(t(key)).toBeTruthy(); + } + }); + + it('exist and resolve in zh-TW and zh-CN without falling back to English', () => { + setMomentLocale('zh-tw'); + for (const key of keys) { + expect(t(key)).toBeTruthy(); + } + expect(t('settings.automaticSync.name')).toBe('自動同步'); + + setMomentLocale('zh-cn'); + for (const key of keys) { + expect(t(key)).toBeTruthy(); + } + expect(t('settings.automaticSync.name')).toBe('自动同步'); + }); + + it('keeps the existing refresh-on-startup setting as a separate key', () => { + setMomentLocale(undefined); + expect(t('settings.autoRefreshOnStartup.name')).toBe('Refresh status on startup'); + expect(t('settings.autoRefreshOnStartup.name')).not.toBe(t('settings.automaticSync.name')); + }); + + it('interpolates the minimum interval into the interval description', () => { + setMomentLocale(undefined); + expect(t('settings.automaticSyncInterval.desc', { min: 1 })).toContain('1'); + }); + }); + describe('inline plural forms ({name|singular|plural})', () => { it('renders the singular branch when the value is exactly 1 and plural otherwise', () => { setMomentLocale(undefined); diff --git a/tests/logic/source-control/AutomaticSyncService.test.ts b/tests/logic/source-control/AutomaticSyncService.test.ts new file mode 100644 index 0000000..785f48b --- /dev/null +++ b/tests/logic/source-control/AutomaticSyncService.test.ts @@ -0,0 +1,145 @@ +import { describe, expect, it, vi } from 'vitest'; +import { AutomaticSyncService } from '../../../src/logic/source-control/AutomaticSyncService'; +import { ChangeRepository } from '../../../src/logic/source-control/ChangeRepository'; +import type { SyncExecutionMode } from '../../../src/logic/source-control/SyncIntentExecutor'; +import type { SyncIntentRequest } from '../../../src/logic/source-control/SyncIntent'; +import { toChangeId } from '../../../src/logic/source-control/types'; +import type { SyncChange, SyncChangeKind } from '../../../src/logic/source-control/types'; +import type { SyncStatusRefreshResult } from '../../../src/logic/sync/SyncStatusRefreshService'; + +function change(path: string, kind: SyncChangeKind): SyncChange { + return { id: toChangeId(path), path, kind }; +} + +function emptyRefreshResult(): SyncStatusRefreshResult { + return {} as SyncStatusRefreshResult; +} + +function buildService(changes: SyncChange[], overrides: { + refresh?: () => Promise; + sync?: (intents: readonly SyncIntentRequest[], mode?: SyncExecutionMode) => Promise; + onError?: (error: unknown) => void; +} = {}) { + const repository = new ChangeRepository(); + repository.replace(changes); + const refresh = vi.fn(overrides.refresh ?? (() => Promise.resolve(emptyRefreshResult()))); + const sync = vi.fn(overrides.sync ?? (() => Promise.resolve(undefined))); + const service = new AutomaticSyncService({ + workspace: { refresh }, + changes: repository, + actions: { sync }, + onError: overrides.onError, + }); + return { service, refresh, sync, repository }; +} + +describe('AutomaticSyncService', () => { + it('refreshes authoritative state before and after execution, and uses the refreshed repository snapshot', async () => { + const order: string[] = []; + const repository = new ChangeRepository(); + const refresh = vi.fn() + .mockImplementation(async () => { + if (!repository.getById(toChangeId('note.md'))) { + repository.replace([change('note.md', 'local-modified')]); + } + order.push('refresh'); + }); + const sync = vi.fn().mockImplementation(async () => { order.push('sync'); }); + const service = new AutomaticSyncService({ + workspace: { refresh }, + changes: repository, + actions: { sync }, + }); + + await service.runOnce(); + + expect(refresh).toHaveBeenCalledTimes(2); + // Intent generation read the repository only after the first refresh. + expect(sync).toHaveBeenCalledTimes(1); + const intents = sync.mock.calls[0]?.[0] as SyncIntentRequest[]; + expect(intents.map(intent => intent.changeId)).toContain(toChangeId('note.md')); + expect(order).toEqual(['refresh', 'sync', 'refresh']); + }); + + it('excludes synced and already-conflicted changes from intent generation', async () => { + const { service, sync } = buildService([ + change('a.md', 'local-only'), + change('b.md', 'remote-only'), + change('synced.md', 'synced'), + change('conflict.md', 'conflict'), + ]); + + await service.runOnce(); + + const intents = sync.mock.calls[0]?.[0] as SyncIntentRequest[]; + const paths = intents.map(intent => intent.changeId); + expect(paths).toEqual([toChangeId('a.md'), toChangeId('b.md')]); + }); + + it('routes each change through the default action policy and executes in background mode', async () => { + const { service, sync } = buildService([ + change('push.md', 'local-modified'), + change('pull.md', 'remote-modified'), + change('delete.md', 'local-deleted'), + change('move.md', 'moved'), + ]); + + await service.runOnce(); + + const [intents, mode] = sync.mock.calls[0] as [SyncIntentRequest[], SyncExecutionMode]; + expect(mode).toBe('background'); + expect(intents).toEqual([ + { changeId: toChangeId('push.md'), action: 'push' }, + { changeId: toChangeId('pull.md'), action: 'pull' }, + { changeId: toChangeId('delete.md'), action: 'delete-remote' }, + { changeId: toChangeId('move.md'), action: 'push' }, + ]); + }); + + it('performs no mutation when there are no pending changes', async () => { + const { service, sync, refresh } = buildService([ + change('synced.md', 'synced'), + change('conflict.md', 'conflict'), + ]); + + await service.runOnce(); + + expect(sync).not.toHaveBeenCalled(); + // Still refreshes before/after so the panel reflects current state. + expect(refresh).toHaveBeenCalledTimes(2); + }); + + it('does not execute concurrently on overlapping runOnce calls; the second tick is skipped', async () => { + let releaseFirst: (() => void) | undefined; + const firstRun = new Promise(resolve => { releaseFirst = resolve; }); + const { service, sync } = buildService([change('a.md', 'local-only')], { + sync: () => firstRun, + }); + + const first = service.runOnce(); + const second = service.runOnce(); + + // The overlapping call returns immediately without a second sync. + await second; + expect(sync).toHaveBeenCalledTimes(1); + + releaseFirst?.(); + await first; + expect(sync).toHaveBeenCalledTimes(1); + }); + + it('is not left permanently locked after an execution error, and reports it', async () => { + const onError = vi.fn(); + const sync = vi.fn() + .mockRejectedValueOnce(new Error('provider down')) + .mockResolvedValueOnce(undefined); + const { service } = buildService([change('a.md', 'local-only')], { sync, onError }); + + await service.runOnce(); + expect(onError).toHaveBeenCalledTimes(1); + + // A later tick still runs and does not swallow the fresh call. + await service.runOnce(); + expect(sync).toHaveBeenCalledTimes(2); + }); +}); diff --git a/tests/logic/source-control/SourceControlActionService.test.ts b/tests/logic/source-control/SourceControlActionService.test.ts index 60b8ac9..219cbc8 100644 --- a/tests/logic/source-control/SourceControlActionService.test.ts +++ b/tests/logic/source-control/SourceControlActionService.test.ts @@ -374,6 +374,138 @@ describe('SourceControlActionService', () => { expect(operations.get(toChangeId('c-1'))).toBe('idle'); }); + describe('background execution mode', () => { + it('does not show the final confirmation modal and auto-accepts the merged plan', async () => { + const confirmPlan = vi.fn().mockResolvedValue(true); + const commitResolvedBatch = vi.fn().mockResolvedValue(undefined); + const notify = vi.fn(); + const planPush = vi.fn().mockResolvedValue(emptyPlannedBatch({ + reviewPlan: emptySyncPlan({ modifications: [{ path: 'a.md', name: 'a.md' }] }), + pushes: [{ path: 'a.md', name: 'a.md', repoPath: 'a.md', content: 'updated', existingSha: 'sha-a' }], + })); + const { service, operations } = buildService( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }], + fakeWorkspace({ planPush, commitResolvedBatch, confirmPlan }), + { notify }, + ); + + await service.sync(intents(toChangeId('c-1')), 'background'); + + expect(confirmPlan).not.toHaveBeenCalled(); + expect(commitResolvedBatch).toHaveBeenCalledTimes(1); + expect(operations.get(toChangeId('c-1'))).toBe('success'); + // Background runs stay silent on success. + expect(notify).not.toHaveBeenCalled(); + }); + + it('requests skip conflict behavior from the planner, not the interactive prompt', async () => { + const planPush = vi.fn().mockResolvedValue(emptyPlannedBatch()); + const { service } = buildService( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }], + fakeWorkspace({ planPush }), + ); + + await service.sync(intents(toChangeId('c-1')), 'background'); + + expect(planPush).toHaveBeenCalledWith(['a.md'], 'skip'); + }); + + it('never marks a path that became a conflict as operation success, even when it was in the target set', async () => { + const commitResolvedBatch = vi.fn().mockResolvedValue(undefined); + const notify = vi.fn(); + // `safe.md` commits fine; `clash.md` was requested but the + // background planner skipped it as a conflict and left it out + // of `pushes`. + const planPush = vi.fn().mockResolvedValue(emptyPlannedBatch({ + reviewPlan: emptySyncPlan({ + modifications: [{ path: 'safe.md', name: 'safe.md' }], + skippedConflicts: [{ path: 'clash.md', name: 'clash.md' }], + }), + pushes: [{ path: 'safe.md', name: 'safe.md', repoPath: 'safe.md', content: 'x', existingSha: 'sha-safe' }], + conflictedPaths: ['clash.md'], + })); + const { service, operations } = buildService( + [ + { id: toChangeId('safe'), path: 'safe.md', kind: 'local-modified' }, + { id: toChangeId('clash'), path: 'clash.md', kind: 'conflict' }, + ], + fakeWorkspace({ planPush, commitResolvedBatch }), + { notify }, + ); + + await service.sync(intents(toChangeId('safe'), toChangeId('clash')), 'background'); + + expect(operations.get(toChangeId('safe'))).toBe('success'); + // The skipped conflict must not be reported as a successful operation. + expect(operations.get(toChangeId('clash'))).not.toBe('success'); + expect(operations.get(toChangeId('clash'))).toBe('idle'); + }); + + it('keeps syncing unrelated safe files when another push path conflicts', async () => { + const commitResolvedBatch = vi.fn().mockResolvedValue(undefined); + const planPush = vi.fn().mockResolvedValue(emptyPlannedBatch({ + reviewPlan: emptySyncPlan({ modifications: [{ path: 'safe.md', name: 'safe.md' }] }), + pushes: [{ path: 'safe.md', name: 'safe.md', repoPath: 'safe.md', content: 'x' }], + conflictedPaths: ['clash.md'], + })); + const { service, operations } = buildService( + [ + { id: toChangeId('safe'), path: 'safe.md', kind: 'local-modified' }, + { id: toChangeId('clash'), path: 'clash.md', kind: 'conflict' }, + ], + fakeWorkspace({ planPush, commitResolvedBatch }), + ); + + await service.sync(intents(toChangeId('safe'), toChangeId('clash')), 'background'); + + expect(commitResolvedBatch).toHaveBeenCalledTimes(1); + expect(operations.get(toChangeId('safe'))).toBe('success'); + }); + }); + + it('serializes a manual push behind an in-flight background run instead of discarding or racing it', async () => { + // Background sync holds the execution guard while its provider + // commit is pending; the manual push must wait for it. + let releaseBackground: (() => void) | undefined; + const backgroundCommit = new Promise(resolve => { releaseBackground = resolve; }); + const order: string[] = []; + + const planPush = vi.fn().mockResolvedValue(emptyPlannedBatch({ + reviewPlan: emptySyncPlan({ modifications: [{ path: 'bg.md', name: 'bg.md' }] }), + pushes: [{ path: 'bg.md', name: 'bg.md', repoPath: 'bg.md', content: 'x' }], + })); + const commitResolvedBatch = vi.fn().mockImplementation(async () => { + order.push('background-commit-start'); + await backgroundCommit; + order.push('background-commit-end'); + }); + const push = vi.fn().mockImplementation(async () => { + order.push('manual-push'); + return emptyPushResults({ syncedPaths: [{ path: 'manual.md' }] }); + }); + + const { service } = buildService( + [ + { id: toChangeId('bg'), path: 'bg.md', kind: 'local-modified' }, + { id: toChangeId('manual'), path: 'manual.md', kind: 'local-only' }, + ], + fakeWorkspace({ planPush, commitResolvedBatch, push }), + ); + + const background = service.sync(intents(toChangeId('bg')), 'background'); + const manual = service.push([toChangeId('manual')]); + + // Let the background planning/commit reach its pending provider + // call; the manual push must not have started yet. + await vi.waitFor(() => expect(order).toEqual(['background-commit-start'])); + + releaseBackground?.(); + await background; + await manual; + + expect(order).toEqual(['background-commit-start', 'background-commit-end', 'manual-push']); + }); + it('counts keep-remote conflict resolutions as acceptedRemote in the single sync notification (full success)', async () => { const notify = vi.fn(); const commitResolvedBatch = vi.fn(async (_pushes, _moves, _deletions, _keepRemote, _keepLocal, results: PushResults) => { diff --git a/tests/logic/source-control/SyncExecutionGuard.test.ts b/tests/logic/source-control/SyncExecutionGuard.test.ts new file mode 100644 index 0000000..fde7f17 --- /dev/null +++ b/tests/logic/source-control/SyncExecutionGuard.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from 'vitest'; +import { SyncExecutionGuard } from '../../../src/logic/source-control/SyncExecutionGuard'; + +describe('SyncExecutionGuard', () => { + it('reports locked while held and unlocks on release', async () => { + const guard = new SyncExecutionGuard(); + expect(guard.isLocked).toBe(false); + + const release = await guard.acquire(); + expect(guard.isLocked).toBe(true); + + release(); + expect(guard.isLocked).toBe(false); + }); + + it('tryAcquire fails (returns null) while held, so automatic work can skip', async () => { + const guard = new SyncExecutionGuard(); + const release = await guard.acquire(); + + expect(guard.tryAcquire()).toBeNull(); + + release(); + expect(guard.tryAcquire()).not.toBeNull(); + }); + + it('queues a waiting acquire so manual work is serialized rather than discarded', async () => { + const guard = new SyncExecutionGuard(); + const release = await guard.acquire(); + + const order: string[] = []; + const waiting = guard.acquire().then(release2 => { + order.push('acquired'); + release2(); + }); + order.push('waiting'); + + release(); + await waiting; + + expect(order).toEqual(['waiting', 'acquired']); + }); + + it('ignores a second release call from the same holder', async () => { + const guard = new SyncExecutionGuard(); + const release = await guard.acquire(); + release(); + release(); + expect(guard.isLocked).toBe(false); + }); +}); diff --git a/tests/logic/sync-manager-mapping.test.ts b/tests/logic/sync-manager-mapping.test.ts index 4277d8f..ae7d8ab 100644 --- a/tests/logic/sync-manager-mapping.test.ts +++ b/tests/logic/sync-manager-mapping.test.ts @@ -65,6 +65,9 @@ const mockSettings: GitLabFilesPushSettings = { bannerDismissedVersion: '', language: 'system', autoRefreshOnStartup: true, + automaticSyncEnabled: false, + automaticSyncIntervalMinutes: 5, + automaticSyncOnStartup: false, }; describe('SyncManager Mapping', () => { diff --git a/tests/logic/sync-manager.test.ts b/tests/logic/sync-manager.test.ts index 7f3fc31..4540533 100644 --- a/tests/logic/sync-manager.test.ts +++ b/tests/logic/sync-manager.test.ts @@ -77,7 +77,10 @@ const mockSettings: GitLabFilesPushSettings = { lastSeenVersion: '', bannerDismissedVersion: '', language: 'system', - autoRefreshOnStartup: true + autoRefreshOnStartup: true, + automaticSyncEnabled: false, + automaticSyncIntervalMinutes: 5, + automaticSyncOnStartup: false, }; describe('SyncManager', () => { diff --git a/tests/logic/sync/PushCoordinator.test.ts b/tests/logic/sync/PushCoordinator.test.ts index c0a7bba..6c73759 100644 --- a/tests/logic/sync/PushCoordinator.test.ts +++ b/tests/logic/sync/PushCoordinator.test.ts @@ -31,12 +31,17 @@ function settings(): GitLabFilesPushSettings { bannerDismissedVersion: '', language: 'system', autoRefreshOnStartup: true, + automaticSyncEnabled: false, + automaticSyncIntervalMinutes: 5, + automaticSyncOnStartup: false, }; } function createHarness(overrides: { confirmPlan?: boolean; pathExists?: (path: string) => Promise; + resolveConflicts?: (conflicts: unknown[], safeCount: number) => Promise; + settings?: GitLabFilesPushSettings; } = {}) { const listFilesDetailed = vi.fn().mockResolvedValue([]); const provider = { @@ -62,7 +67,8 @@ function createHarness(overrides: { }); const saveSettings = vi.fn().mockResolvedValue(undefined); const confirmPlan = vi.fn().mockResolvedValue(overrides.confirmPlan ?? true); - const syncSettings = settings(); + const resolveConflicts = vi.fn(overrides.resolveConflicts ?? (() => Promise.resolve(true))); + const syncSettings = overrides.settings ?? settings(); const coordinator = new PushCoordinator({ app: { vault: { getFileByPath: vi.fn().mockReturnValue({}) } } as unknown as App, gitService: () => provider, @@ -72,14 +78,28 @@ function createHarness(overrides: { conflicts: { findStale: vi.fn().mockResolvedValue([]), applyRemote: vi.fn() } as unknown as ConflictResolver, isPathIgnored: () => false, confirmPlan, - resolveConflicts: vi.fn().mockResolvedValue(true), + resolveConflicts, updateMetadata: vi.fn().mockResolvedValue(undefined), migrateBaseline: vi.fn().mockResolvedValue(undefined), saveSettings, notify: vi.fn(), serviceName: () => 'GitHub', }); - return { coordinator, listFilesDetailed, commitBatch, confirmPlan, saveSettings, settings: syncSettings }; + return { coordinator, listFilesDetailed, commitBatch, confirmPlan, resolveConflicts, saveSettings, settings: syncSettings }; +} + +/** + * One conflicted candidate (`clash.md`, tracked baseline + divergent remote) + * alongside one safe candidate (`safe.md`, no tracked baseline). + */ +function conflictingSettings(): GitLabFilesPushSettings { + const base = settings(); + base.syncMetadata['clash.md'] = { + lastSyncedSha: 'base-clash', + lastSyncedAt: 0, + lastKnownPath: 'clash.md', + }; + return base; } describe('PushCoordinator', () => { @@ -124,6 +144,49 @@ describe('PushCoordinator', () => { expect(harness.commitBatch).not.toHaveBeenCalled(); }); + describe('planSyncBatch conflict behavior', () => { + function conflictedHarness(resolveConflicts?: (conflicts: unknown[], safeCount: number) => Promise) { + const harness = createHarness({ settings: conflictingSettings(), resolveConflicts }); + harness.listFilesDetailed.mockResolvedValue([ + { path: 'clash.md', symlink: false, sha: 'remote-clash' }, + ]); + return harness; + } + + it('prompts for conflict resolution in interactive mode and cancels the batch when declined', async () => { + const harness = conflictedHarness(() => Promise.resolve(false)); + + const plan = await harness.coordinator.planSyncBatch( + ['safe.md', 'clash.md'], + undefined, + undefined, + 'prompt', + ); + + expect(harness.resolveConflicts).toHaveBeenCalledTimes(1); + expect(plan.cancelled).toBe(true); + }); + + it('skips conflicting paths and continues with safe paths in background mode, invoking no conflict UI', async () => { + const resolveConflicts = vi.fn().mockResolvedValue(true); + const harness = conflictedHarness(resolveConflicts); + + const plan = await harness.coordinator.planSyncBatch( + ['safe.md', 'clash.md'], + undefined, + undefined, + 'skip', + ); + + expect(resolveConflicts).not.toHaveBeenCalled(); + expect(plan.cancelled).toBe(false); + expect(plan.conflictedPaths).toContain('clash.md'); + expect(plan.skippedConflicts).toBeGreaterThan(0); + expect(plan.pushes.map(entry => entry.path)).not.toContain('clash.md'); + expect(plan.pushes.map(entry => entry.path)).toContain('safe.md'); + }); + }); + it('plans an edited tracked rename as a move when the destination is free', async () => { const harness = createHarness(); harness.settings.syncMetadata['notes/new.md'] = { diff --git a/tests/main.test.ts b/tests/main.test.ts index aa7e14a..35d083e 100644 --- a/tests/main.test.ts +++ b/tests/main.test.ts @@ -37,6 +37,71 @@ describe('GitLabFilesPush.trackFolderRename', () => { expect(handleFileRenamed).toHaveBeenCalledTimes(2); }); + describe('startup behavior', () => { + function callLayoutReady(fakePlugin: Record): void { + (GitLabFilesPush.prototype as unknown as { + handleLayoutReady(this: unknown): void; + }).handleLayoutReady.call(fakePlugin); + } + + it('runs background automatic sync and does NOT open Source Control when enabled + on startup', () => { + const runOnce = vi.fn().mockResolvedValue(undefined); + const refresh = vi.fn().mockResolvedValue(undefined); + const activateSourceControlView = vi.fn().mockResolvedValue(undefined); + const fakePlugin = { + settings: { automaticSyncEnabled: true, automaticSyncOnStartup: true, autoRefreshOnStartup: true }, + automaticSync: { runOnce }, + sourceControlViewModel: { refresh }, + activateSourceControlView, + normalizeSourceControlLeaves: vi.fn(), + }; + + callLayoutReady(fakePlugin); + + expect(runOnce).toHaveBeenCalledTimes(1); + // No redundant legacy startup refresh / view reveal. + expect(activateSourceControlView).not.toHaveBeenCalled(); + expect(refresh).not.toHaveBeenCalled(); + }); + + it('falls back to the existing refresh-on-startup behavior when automatic sync is off', () => { + const runOnce = vi.fn().mockResolvedValue(undefined); + const fakePlugin = { + settings: { automaticSyncEnabled: false, automaticSyncOnStartup: true, autoRefreshOnStartup: true }, + automaticSync: { runOnce }, + // refreshSyncStatusOnStartup is a prototype method; stub the two + // calls it makes on this object. + sourceControlViewModel: { refresh: vi.fn().mockResolvedValue(undefined) }, + activateSourceControlView: vi.fn().mockResolvedValue(undefined), + normalizeSourceControlLeaves: vi.fn(), + }; + (fakePlugin as Record).refreshSyncStatusOnStartup = + (GitLabFilesPush.prototype as unknown as Record).refreshSyncStatusOnStartup; + + callLayoutReady(fakePlugin); + + expect(runOnce).not.toHaveBeenCalled(); + expect(fakePlugin.activateSourceControlView).toHaveBeenCalledTimes(1); + }); + + it('does nothing at startup when both automatic sync and refresh-on-startup are off', () => { + const runOnce = vi.fn(); + const activateSourceControlView = vi.fn(); + const fakePlugin = { + settings: { automaticSyncEnabled: false, automaticSyncOnStartup: false, autoRefreshOnStartup: false }, + automaticSync: { runOnce }, + sourceControlViewModel: { refresh: vi.fn() }, + activateSourceControlView, + normalizeSourceControlLeaves: vi.fn(), + }; + + callLayoutReady(fakePlugin); + + expect(runOnce).not.toHaveBeenCalled(); + expect(activateSourceControlView).not.toHaveBeenCalled(); + }); + }); + it('does nothing when no files live under the moved folder', async () => { const trackRename = vi.fn().mockResolvedValue(undefined); const fakePlugin = { diff --git a/tests/runtime/AutomaticSyncScheduler.test.ts b/tests/runtime/AutomaticSyncScheduler.test.ts new file mode 100644 index 0000000..4691182 --- /dev/null +++ b/tests/runtime/AutomaticSyncScheduler.test.ts @@ -0,0 +1,129 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { AutomaticSyncScheduler } from '../../src/runtime/AutomaticSyncScheduler'; + +interface Settings { + automaticSyncEnabled: boolean; + automaticSyncIntervalMinutes: number; +} + +function buildScheduler(initial: Settings, run = vi.fn().mockResolvedValue(undefined)) { + let settings: Settings = { ...initial }; + const registerInterval = vi.fn((id: number) => id); + const scheduler = new AutomaticSyncScheduler({ + getSettings: () => settings, + run, + registerInterval, + }); + return { + scheduler, + run, + registerInterval, + setSettings(next: Partial) { settings = { ...settings, ...next }; }, + }; +} + +describe('AutomaticSyncScheduler', () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it('does not schedule anything while automatic sync is disabled', () => { + vi.useFakeTimers(); + const harness = buildScheduler({ automaticSyncEnabled: false, automaticSyncIntervalMinutes: 5 }); + + harness.scheduler.apply(); + + expect(harness.scheduler.isScheduled).toBe(false); + vi.advanceTimersByTime(60 * 60_000); + expect(harness.run).not.toHaveBeenCalled(); + }); + + it('runs on the configured interval when enabled', async () => { + vi.useFakeTimers(); + const harness = buildScheduler({ automaticSyncEnabled: true, automaticSyncIntervalMinutes: 5 }); + + harness.scheduler.apply(); + expect(harness.scheduler.isScheduled).toBe(true); + + await vi.advanceTimersByTimeAsync(5 * 60_000); + expect(harness.run).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(5 * 60_000); + expect(harness.run).toHaveBeenCalledTimes(2); + }); + + it('clears the old interval and installs the new one when the interval changes', async () => { + vi.useFakeTimers(); + const harness = buildScheduler({ automaticSyncEnabled: true, automaticSyncIntervalMinutes: 5 }); + harness.scheduler.apply(); + + harness.setSettings({ automaticSyncIntervalMinutes: 10 }); + harness.scheduler.apply(); + + // The old 5-minute tick no longer fires. + await vi.advanceTimersByTimeAsync(5 * 60_000); + expect(harness.run).not.toHaveBeenCalled(); + + // The new 10-minute schedule is active. + await vi.advanceTimersByTimeAsync(5 * 60_000); + expect(harness.run).toHaveBeenCalledTimes(1); + }); + + it('stops scheduled execution when disabled again', async () => { + vi.useFakeTimers(); + const harness = buildScheduler({ automaticSyncEnabled: true, automaticSyncIntervalMinutes: 5 }); + harness.scheduler.apply(); + + harness.setSettings({ automaticSyncEnabled: false }); + harness.scheduler.apply(); + + expect(harness.scheduler.isScheduled).toBe(false); + await vi.advanceTimersByTimeAsync(30 * 60_000); + expect(harness.run).not.toHaveBeenCalled(); + }); + + it('does not postpone the existing timer when unrelated settings are re-applied unchanged', async () => { + vi.useFakeTimers(); + const harness = buildScheduler({ automaticSyncEnabled: true, automaticSyncIntervalMinutes: 5 }); + harness.scheduler.apply(); + + await vi.advanceTimersByTimeAsync(4 * 60_000); + harness.scheduler.apply(); + await vi.advanceTimersByTimeAsync(1 * 60_000); + + expect(harness.run).toHaveBeenCalledTimes(1); + }); + + it('never creates a zero/rapid timer from an invalid interval, falling back to the default', async () => { + vi.useFakeTimers(); + const harness = buildScheduler({ automaticSyncEnabled: true, automaticSyncIntervalMinutes: 0 }); + harness.scheduler.apply(); + + // 0 falls back to the 5-minute default. + await vi.advanceTimersByTimeAsync(1_000); + expect(harness.run).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(5 * 60_000 - 1_000); + expect(harness.run).toHaveBeenCalledTimes(1); + }); + + it('dispose stops scheduled execution and clears registration state', async () => { + vi.useFakeTimers(); + const harness = buildScheduler({ automaticSyncEnabled: true, automaticSyncIntervalMinutes: 1 }); + harness.scheduler.apply(); + + harness.scheduler.dispose(); + + expect(harness.scheduler.isScheduled).toBe(false); + await vi.advanceTimersByTimeAsync(10 * 60_000); + expect(harness.run).not.toHaveBeenCalled(); + }); + + it('registers the interval with the plugin lifecycle so unload clears it', () => { + vi.useFakeTimers(); + const harness = buildScheduler({ automaticSyncEnabled: true, automaticSyncIntervalMinutes: 5 }); + + harness.scheduler.apply(); + + expect(harness.registerInterval).toHaveBeenCalledTimes(1); + }); +}); diff --git a/tests/runtime/createSyncRuntime.test.ts b/tests/runtime/createSyncRuntime.test.ts index f6eb8e3..6ee05e9 100644 --- a/tests/runtime/createSyncRuntime.test.ts +++ b/tests/runtime/createSyncRuntime.test.ts @@ -60,6 +60,28 @@ describe('createSyncRuntime', () => { expect(runtime.refreshState).toBeDefined(); expect(runtime.sourceControlViewModel).toBeDefined(); expect(runtime.sourceControlActions).toBeDefined(); + expect(runtime.automaticSync).toBeDefined(); + }); + + it('wires AutomaticSyncService to refresh, the shared ChangeRepository, and the background sync path', async () => { + const runtime = createSyncRuntime(buildDeps()); + // Stub the workspace refresh (the real service needs a full vault mock) + // and the sync execution, isolating the wiring under test. + const refresh = vi.spyOn(runtime.syncWorkspace, 'refresh').mockResolvedValue({ + statuses: new Map(), + remoteEntries: [], + } as never); + const sync = vi.spyOn(runtime.sourceControlActions, 'sync').mockResolvedValue(undefined); + + runtime.sync.status.set({ path: 'note.md', status: 'unsynced' }); + await runtime.automaticSync.runOnce(); + + expect(refresh).toHaveBeenCalledTimes(2); + expect(sync).toHaveBeenCalledTimes(1); + expect(sync).toHaveBeenCalledWith( + [expect.objectContaining({ changeId: 'note.md' })], + 'background', + ); }); it('keeps ChangeRepository in sync with the shared SyncStatusService until disposed', () => { diff --git a/tests/settings.test.ts b/tests/settings.test.ts index 3fcce73..3e2b299 100644 --- a/tests/settings.test.ts +++ b/tests/settings.test.ts @@ -34,9 +34,54 @@ describe('settings module split', () => { bannerDismissedVersion: '', language: 'system', autoRefreshOnStartup: true, + automaticSyncEnabled: false, + automaticSyncIntervalMinutes: 5, + automaticSyncOnStartup: false, }); }); + it('defaults automatic sync to OFF, 5 minutes, and startup sync OFF', () => { + expect(settingsModel.DEFAULT_SETTINGS.automaticSyncEnabled).toBe(false); + expect(settingsModel.DEFAULT_SETTINGS.automaticSyncIntervalMinutes).toBe(5); + expect(settingsModel.DEFAULT_SETTINGS.automaticSyncOnStartup).toBe(false); + }); + + it('merges older stored settings with the new automatic-sync defaults', () => { + // A settings object persisted before automatic sync existed. + const stored = { gitlabToken: 'abc', branch: 'develop' } as Partial; + const merged = { ...settingsModel.DEFAULT_SETTINGS, ...stored }; + expect(merged.automaticSyncEnabled).toBe(false); + expect(merged.automaticSyncIntervalMinutes).toBe(5); + expect(merged.automaticSyncOnStartup).toBe(false); + expect(merged.branch).toBe('develop'); + }); +}); + +describe('normalizeAutomaticSyncIntervalMinutes', () => { + const fallback = settingsModel.DEFAULT_SETTINGS.automaticSyncIntervalMinutes; + + it('accepts a finite interval at or above the minimum', () => { + expect(settingsHelpers.normalizeAutomaticSyncIntervalMinutes(1, fallback)).toBe(1); + expect(settingsHelpers.normalizeAutomaticSyncIntervalMinutes(45, fallback)).toBe(45); + }); + + it('floors fractional minute values', () => { + expect(settingsHelpers.normalizeAutomaticSyncIntervalMinutes(2.9, fallback)).toBe(2); + }); + + it('rejects zero, negative, NaN, Infinity, and non-numeric input so no rapid timer can be created', () => { + expect(settingsHelpers.normalizeAutomaticSyncIntervalMinutes(0, fallback)).toBe(fallback); + expect(settingsHelpers.normalizeAutomaticSyncIntervalMinutes(-5, fallback)).toBe(fallback); + expect(settingsHelpers.normalizeAutomaticSyncIntervalMinutes(Number.NaN, fallback)).toBe(fallback); + expect(settingsHelpers.normalizeAutomaticSyncIntervalMinutes(Number.POSITIVE_INFINITY, fallback)).toBe(fallback); + expect(settingsHelpers.normalizeAutomaticSyncIntervalMinutes('nonsense', fallback)).toBe(fallback); + expect(settingsHelpers.normalizeAutomaticSyncIntervalMinutes(undefined, fallback)).toBe(fallback); + }); + + it('parses a numeric string from older storage', () => { + expect(settingsHelpers.normalizeAutomaticSyncIntervalMinutes('10', fallback)).toBe(10); + }); + 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'); diff --git a/tests/setup.ts b/tests/setup.ts index 63c8eb3..b141067 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -19,11 +19,11 @@ if (typeof document === 'undefined') { } if (typeof window === 'undefined') { (globalThis as unknown as { window: unknown }).window = { - setInterval: vi.fn(), - clearInterval: vi.fn(), // Delegate to the global timers (not bound methods captured once) so // vi.useFakeTimers()/vi.useRealTimers() — which patch globalThis — keep - // controlling window.setTimeout/clearTimeout too. + // controlling window.setTimeout/clearTimeout/setInterval too. + setInterval: (handler: (...args: unknown[]) => void, timeout?: number) => globalThis.setInterval(handler, timeout), + clearInterval: (handle?: ReturnType) => globalThis.clearInterval(handle), setTimeout: (handler: (...args: unknown[]) => void, timeout?: number) => globalThis.setTimeout(handler, timeout), clearTimeout: (handle?: ReturnType) => globalThis.clearTimeout(handle), }; @@ -66,6 +66,8 @@ class BaseTextComponent { onChange(handler: (value: string) => void) { this.changeHandler = handler; + this.inputEl.addEventListener('input', () => handler(this.inputEl.value)); + this.inputEl.addEventListener('change', () => handler(this.inputEl.value)); return this; } @@ -78,6 +80,7 @@ class BaseTextComponent { export const TextComponent = class extends BaseTextComponent { constructor(containerEl?: HTMLElement) { const inputEl = document.createElement('input'); + inputEl.type = 'text'; containerEl?.appendChild(inputEl); super(inputEl); } @@ -157,31 +160,103 @@ export const ButtonComponent = class { export const ExtraButtonComponent = class extends ButtonComponent {}; +export const ToggleComponent = class { + toggleEl: HTMLInputElement; + private changeHandler?: (value: boolean) => void; + + constructor(containerEl?: HTMLElement) { + const inputEl = document.createElement('input'); + inputEl.type = 'checkbox'; + containerEl?.appendChild(inputEl); + this.toggleEl = inputEl; + } + + setValue(value: boolean) { + this.toggleEl.checked = value; + return this; + } + + setDisabled(disabled: boolean) { + this.toggleEl.disabled = disabled; + return this; + } + + onChange(handler: (value: boolean) => void) { + this.changeHandler = handler; + this.toggleEl.addEventListener('change', () => handler(this.toggleEl.checked)); + return this; + } + + triggerChange(value: boolean) { + this.toggleEl.checked = value; + this.changeHandler?.(value); + } + + getValue() { + return this.toggleEl.checked; + } +}; + export const Setting = class { containerEl?: HTMLElement; + settingEl?: HTMLElement; + private disabled = false; constructor(containerEl?: HTMLElement) { this.containerEl = containerEl; + // Build a faithful `.setting-item` row (name/desc/control) so tests can + // find a control by its row label, like the real Obsidian DOM. + if (containerEl) { + const row = document.createElement('div'); + row.className = 'setting-item'; + this.settingEl = row; + containerEl.appendChild(row); + } + } + + private controlContainer(): HTMLElement | undefined { + return this.settingEl ? this.settingEl.appendChild(document.createElement('div')) : this.containerEl; + } + + setName(name: string) { + if (!this.settingEl) return this; + const el = document.createElement('div'); + el.className = 'setting-item-name'; + el.textContent = name; + this.settingEl.appendChild(el); + return this; + } + + setDesc(desc: string) { + if (!this.settingEl) return this; + const el = document.createElement('div'); + el.className = 'setting-item-description'; + el.textContent = desc; + this.settingEl.appendChild(el); + return this; } - setName() { return this; } - setDesc() { return this; } setHeading() { return this; } - addToggle() { return this; } + setDisabled(disabled: boolean) { this.disabled = disabled; return this; } + isDisabled() { return this.disabled; } + addToggle(callback?: (component: InstanceType) => void) { + if (callback) callback(new ToggleComponent(this.controlContainer())); + return this; + } addText(callback?: (component: InstanceType) => void) { - if (callback) callback(new TextComponent(this.containerEl)); + if (callback) callback(new TextComponent(this.controlContainer())); return this; } addTextArea(callback?: (component: InstanceType) => void) { - if (callback) callback(new TextAreaComponent(this.containerEl)); + if (callback) callback(new TextAreaComponent(this.controlContainer())); return this; } addButton(callback?: (component: InstanceType) => void) { - if (callback) callback(new ButtonComponent(this.containerEl)); + if (callback) callback(new ButtonComponent(this.controlContainer())); return this; } addExtraButton(callback?: (component: InstanceType) => void) { - if (callback) callback(new ExtraButtonComponent(this.containerEl)); + if (callback) callback(new ExtraButtonComponent(this.controlContainer())); return this; } addDropdown(callback?: (component: InstanceType) => void) { @@ -399,6 +474,7 @@ vi.mock('obsidian', () => ({ DropdownComponent, ButtonComponent, ExtraButtonComponent, + ToggleComponent, requestUrl, setTooltip, setIcon, diff --git a/tests/ui/SettingsAutomaticSync.test.ts b/tests/ui/SettingsAutomaticSync.test.ts new file mode 100644 index 0000000..9c29b53 --- /dev/null +++ b/tests/ui/SettingsAutomaticSync.test.ts @@ -0,0 +1,148 @@ +import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; +import { App } from 'obsidian'; +import { DEFAULT_SETTINGS, GitLabSyncSettingTab } from '../../src/settings'; +import GitLabFilesPush, { type ConnectionStatus } from '../../src/main'; +import type { ConnectionTestResult } from '../../src/services/git-service-interface'; +import { createContainer, setupObsidianDOM } from './setup-dom'; + +vi.mock('../../src/main', () => ({ + default: class {}, +})); + +beforeAll(() => { setupObsidianDOM(); }); + +interface PluginStub { + plugin: GitLabFilesPush; + saveSettings: ReturnType; +} + +function createPluginStub(): PluginStub { + const saveSettings = vi.fn().mockResolvedValue(undefined); + const plugin = { + settings: { ...DEFAULT_SETTINGS }, + manifest: { version: '0.0.0-test' }, + saveSettings, + initializeGitService: vi.fn(), + testConnection: vi.fn().mockResolvedValue({ repoOk: true, branchOk: true } satisfies ConnectionTestResult), + onConnectionStatusChange: vi.fn((listener: (status: ConnectionStatus) => void) => { + listener({ state: 'checking' }); + return () => undefined; + }), + } as unknown as GitLabFilesPush; + return { plugin, saveSettings }; +} + +function renderTab(overrides: Partial = {}): { + tab: GitLabSyncSettingTab; + plugin: GitLabFilesPush; + saveSettings: ReturnType; +} { + vi.useFakeTimers(); + const { plugin, saveSettings } = createPluginStub(); + plugin.settings = { ...plugin.settings, ...overrides }; + const tab = new GitLabSyncSettingTab(new App(), plugin, plugin); + tab.containerEl = createContainer(); + tab.display(); + return { tab, plugin, saveSettings }; +} + +/** Finds the toggle input whose surrounding Setting row has the given name. */ +function toggleByName(tab: GitLabSyncSettingTab, name: string): HTMLInputElement | null { + const inputs = Array.from(tab.containerEl.querySelectorAll('input[type="checkbox"]')); + for (const input of inputs) { + const row = input.closest('.setting-item'); + if (row?.textContent?.includes(name)) return input; + } + return null; +} + +/** The Automatic sync interval text input (placeholder mirrors the 5-minute default). */ +function intervalInputByName(tab: GitLabSyncSettingTab): HTMLInputElement | undefined { + return Array.from(tab.containerEl.querySelectorAll('input[type="text"]')) + .find(input => input.placeholder === '5'); +} + +/** Flips a checkbox the way a user click would (JSDOM click() misses change events). */ +function flipToggle(input: HTMLInputElement): void { + input.checked = !input.checked; + input.dispatchEvent(new Event('change')); +} + +describe('GitLabSyncSettingTab automatic sync', () => { + afterEach(() => { + vi.clearAllTimers(); + vi.useRealTimers(); + }); + + it('defaults automatic sync OFF, interval 5 minutes, and startup sync OFF', () => { + const { plugin } = renderTab(); + expect(plugin.settings.automaticSyncEnabled).toBe(false); + expect(plugin.settings.automaticSyncIntervalMinutes).toBe(5); + expect(plugin.settings.automaticSyncOnStartup).toBe(false); + }); + + it('renders the Automatic sync, Sync interval, and Sync on startup rows plus the separate refresh row', () => { + const { tab } = renderTab(); + + const text = tab.containerEl.textContent ?? ''; + // These strings come from en.ts (default locale in tests). + expect(text).toContain('Automatic sync'); + expect(text).toContain('Sync interval (minutes)'); + expect(text).toContain('Sync on startup'); + // The existing refresh setting stays a distinct, separately-labeled control. + expect(text).toContain('Refresh status on startup'); + }); + + it('persists an enabled automatic sync toggle through saveSettings', () => { + const { tab, plugin, saveSettings } = renderTab(); + const toggle = toggleByName(tab, 'Automatic sync'); + expect(toggle).not.toBeNull(); + + flipToggle(toggle!); + + expect(plugin.settings.automaticSyncEnabled).toBe(true); + expect(saveSettings).toHaveBeenCalled(); + }); + + it('seeds the interval control with the saved value and sanitizes invalid input to the default', () => { + const { tab, plugin } = renderTab({ automaticSyncEnabled: true, automaticSyncIntervalMinutes: 15 }); + const intervalInput = intervalInputByName(tab); + expect(intervalInput?.value).toBe('15'); + + intervalInput!.value = '0'; + intervalInput!.dispatchEvent(new Event('input')); + expect(plugin.settings.automaticSyncIntervalMinutes).toBe(5); + }); + + it('clamps the interval to the minimum of 1 minute', () => { + const { tab, plugin } = renderTab({ automaticSyncEnabled: true, automaticSyncIntervalMinutes: 5 }); + const intervalInput = intervalInputByName(tab); + + intervalInput!.value = '1'; + intervalInput!.dispatchEvent(new Event('input')); + expect(plugin.settings.automaticSyncIntervalMinutes).toBe(1); + }); + + it('persists the Sync on startup toggle', () => { + const { tab, plugin, saveSettings } = renderTab({ automaticSyncEnabled: true }); + const toggle = toggleByName(tab, 'Sync on startup'); + expect(toggle).not.toBeNull(); + + flipToggle(toggle!); + + expect(plugin.settings.automaticSyncOnStartup).toBe(true); + expect(saveSettings).toHaveBeenCalled(); + }); + + it('does not change the existing refresh status on startup semantics', () => { + const { tab, plugin } = renderTab({ autoRefreshOnStartup: true }); + const toggle = toggleByName(tab, 'Refresh status on startup'); + expect(toggle).not.toBeNull(); + + flipToggle(toggle!); + + expect(plugin.settings.autoRefreshOnStartup).toBe(false); + // The automatic-sync fields remain untouched by this control. + expect(plugin.settings.automaticSyncEnabled).toBe(false); + }); +}); diff --git a/tests/ui/SettingsConnectionStatus.test.ts b/tests/ui/SettingsConnectionStatus.test.ts index 8508b41..88ca856 100644 --- a/tests/ui/SettingsConnectionStatus.test.ts +++ b/tests/ui/SettingsConnectionStatus.test.ts @@ -184,4 +184,19 @@ describe('GitLabSyncSettingTab what\'s new banner', () => { const buttons = Array.from(tab.containerEl.querySelectorAll('button')); expect(buttons.some(button => button.textContent === 'View release history')).toBe(true); }); + + it('surfaces the 1.7.0 notable entries in the banner when the manifest version is 1.7.0', () => { + const tab = renderTab('1.7.0'); + const banner = tab.containerEl.querySelector('.gfs-whats-new-banner'); + expect(banner).not.toBeNull(); + expect(banner?.textContent).toContain('1.7.0'); + // At least one user-facing automatic-sync highlight is shown. + const items = Array.from(tab.containerEl.querySelectorAll('.gfs-whats-new-banner-list li')); + expect(items.some(item => item.textContent?.includes('Automatic sync'))).toBe(true); + }); + + it('hides the 1.7.0 banner once dismissed for that exact version', () => { + const tab = renderTab('1.7.0', '1.7.0'); + expect(tab.containerEl.querySelector('.gfs-whats-new-banner')).toBeNull(); + }); }); From b8a806ee0aa96a973d85192ea80bc8a097d9c534 Mon Sep 17 00:00:00 2001 From: tianyao Date: Mon, 21 Sep 2026 02:33:29 +0000 Subject: [PATCH 3/7] fix(sync): make automatic sync failures observable, skip busy ticks, drop idle refresh - SyncIntentExecutor returns SyncExecutionOutcome so background runs expose planning/commit/pull rejections and provider errors to AutomaticSyncService.onError - runBackground holds the shared SyncExecutionGuard across refresh -> execute -> refresh; a busy tick does no refresh, planning, or mutation and is not queued - skip the second refresh when there is nothing actionable - update architecture/progress docs; PR #156 is a combined PR Co-Authored-By: Claude Sonnet 5 --- docs/architecture.md | 31 +- progress.md | 13 +- .../source-control/AutomaticSyncService.ts | 61 +++- .../SourceControlActionService.ts | 40 ++- .../source-control/SyncIntentExecutor.ts | 64 +++- .../source-control/SourceControlItemView.ts | 4 +- .../AutomaticSyncIntegration.test.ts | 307 ++++++++++++++++++ .../AutomaticSyncService.test.ts | 110 ++++++- .../SourceControlActionService.test.ts | 4 +- tests/runtime/createSyncRuntime.test.ts | 10 +- 10 files changed, 573 insertions(+), 71 deletions(-) create mode 100644 tests/logic/source-control/AutomaticSyncIntegration.test.ts diff --git a/docs/architecture.md b/docs/architecture.md index 3d61775..659d059 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -35,8 +35,8 @@ The dependency direction should normally flow downward. Results and state flow b | Application | `SourceControlViewModel` | read-only projection of application state for UI | repository, selection, operation/refresh state | side effects, provider calls, filesystem writes | | Application | `SourceControlActionService` | stable UI-facing facade for immediate Source Control commands | `SyncWorkspace`, `SyncIntentExecutor` | provider-specific logic, duplicated sync planning | | Application | `SyncIntentExecutor` | one Sync Queue workflow: resolve intent, plan, confirm, execute, aggregate; selects interactive vs background execution policy per run | repository, action policy, `SyncWorkspace`, notifier, `SyncExecutionGuard` | UI DOM, provider API implementation | -| Application | `AutomaticSyncService` | one scheduled automatic sync run: refresh, read repository, build default intents, execute in background, refresh again | `SyncWorkspace`, `ChangeRepository`, `SourceControlActionService` | timer/scheduling mechanics, UI rendering, classification rules | -| Application | `SyncExecutionGuard` | application-level serialization of provider mutations (manual waits, automatic try-acquires) | `SyncIntentExecutor`, `SourceControlActionService` | provider calls, scheduling | +| Application | `AutomaticSyncService` | one scheduled automatic sync run inside one guard hold: refresh, read repository, build default intents, execute in background, refresh once more only if something executed; reports execution failures to the diagnostic logger | `SyncWorkspace`, `ChangeRepository`, `SourceControlActionService` | timer/scheduling mechanics, UI rendering, classification rules | +| Application | `SyncExecutionGuard` | the single application-level lock serializing provider mutations (manual waits, automatic try-acquires); owned by `SourceControlActionService`, shared with `SyncIntentExecutor` | `SyncIntentExecutor`, `SourceControlActionService` | provider calls, scheduling | | Plugin runtime | `AutomaticSyncScheduler` (`src/runtime/AutomaticSyncScheduler.ts`) | the automatic-sync interval timer and its lifecycle registration | settings, `AutomaticSyncService`, Obsidian `registerInterval` | sync execution, change classification | | Boundary | `SyncWorkspace` | application-to-sync execution boundary | `SyncManager`, refresh service, diff service | Source Control rendering | | Sync domain | `SyncManager` | compatibility/domain facade for sync operations | coordinators, executors, metadata/status services | Source Control UI state | @@ -127,19 +127,24 @@ AutomaticSyncScheduler (timer, plugin runtime) ↓ AutomaticSyncService.runOnce() ↓ -refresh authoritative local + remote state - ↓ -ChangeRepository → exclude synced/conflict → default intents via ChangeActionPolicy - ↓ -SourceControlActionService.sync(intents, 'background') - ↓ -SyncIntentExecutor (skip-conflict planning, no confirmation) - ↓ -SyncWorkspace → Sync domain → provider - ↓ -refresh status again +SourceControlActionService.runBackground() ← try-acquire shared SyncExecutionGuard + ├─ busy → skip the whole tick (no refresh, no planning, no mutation, no queue) + └─ held for the whole transaction: + refresh authoritative local + remote state + ↓ + ChangeRepository → exclude synced/conflict → default intents via ChangeActionPolicy + ↓ (no intents → stop; exactly one refresh) + session.sync(intents) → SyncIntentExecutor.executeHeld (background: skip-conflict planning, no confirmation) + ↓ + SyncWorkspace → Sync domain → provider + ↓ + report SyncExecutionOutcome failures via onError (logger), never a Notice + ↓ + refresh status once more ``` +Refresh counts: busy tick 0, idle / only synced+conflict 1, executed run 2. + Automatic sync reuses the same application → `SyncWorkspace` → domain → provider path as manual Sync. It does not own classification, rename detection, action routing, conflict algorithms, push/pull planning, or provider mutation logic, and it never calls a concrete provider service directly. ## 4. Architecture rules diff --git a/progress.md b/progress.md index a8c305d..fee3e98 100644 --- a/progress.md +++ b/progress.md @@ -4,13 +4,15 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont ## Current State -**Last Updated:** 2026-09-12 +**Last Updated:** 2026-09-21 **Active Feature:** Issue #141 — Automatic Syncing (v1.7.0). Implementation complete, verified locally; PR pending. -**Branch / PR:** `claude/automatic-sync-141`, a child branch of `claude/mobile-source-control-density` (PR #156, still open). The #141 PR is stacked on that baseline; retarget to `main` only if #156 merges first. PR title: `feat(sync): add automatic scheduled sync` (semantic-release owns the 1.7.0 bump). +**Branch / PR:** `claude/mobile-source-control-density` / PR #156. #156 is now a combined PR: Mobile Source Control density (CSS + structural tests) **and** Automatic Sync (#141, merged in via #159). It is no longer CSS-only. semantic-release owns the 1.7.0 bump. **What landed (#141):** persisted `automaticSyncEnabled` / `automaticSyncIntervalMinutes` / `automaticSyncOnStartup` (defaults OFF / 5 / OFF, interval min 1); settings UI rows distinct from the existing `autoRefreshOnStartup`; EN/zh-TW/zh-CN strings; `AutomaticSyncService` (refresh → repository → default intents → background execute → refresh) wired through `createSyncRuntime`; `AutomaticSyncScheduler` in plugin runtime; `SyncExecutionMode` per-execution policy with `PushConflictBehavior = 'skip'` at the `PushCoordinator` planning boundary; `SyncExecutionGuard` serialization; startup sync that never opens Source Control and supersedes the legacy startup refresh; hand-curated 1.7.0 What's New entry. -**Next:** open the stacked PR (body includes `Closes #141`), then monitor CI. Manual Obsidian verification not performed in this environment (no executable Obsidian runtime) — checklist is in the PR body. +**Review fixes (2026-09-21):** background failures now reach `onError` via `SyncExecutionOutcome`; a busy tick is skipped before any refresh/planning via `SourceControlActionService.runBackground` (one shared `SyncExecutionGuard`, held across refresh → execute → refresh); the redundant second refresh on an idle vault is gone (idle/synced+conflict-only = 1 refresh, executed run = 2, busy = 0). + +**Next:** monitor CI on #156. Manual Obsidian verification not performed in this environment (no executable Obsidian runtime) — checklist is in the PR body. 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. @@ -21,6 +23,11 @@ Below that: the previous "Outstanding Items"/"Verification Evidence" entries tra ## Verification Evidence +2026-09-21 review fixes (Automatic Sync observability / busy skip / single refresh): + +- `npx eslint .` — 0 errors. `npm run build` (tsc + Obsidian 1.11.0 compat + esbuild) — passed. `npx vitest run` — 81 files / 1030 tests passed (new: `tests/logic/source-control/AutomaticSyncIntegration.test.ts`, 14 tests over the real action service). +- Provider E2E (`npm run test:e2e`) **not run**: no provider credentials in this environment. + This session (Issue #141 — Automatic Syncing, `claude/automatic-sync-141` stacked on `claude/mobile-source-control-density`): - `npx eslint .` — 0 errors, 0 warnings. diff --git a/src/logic/source-control/AutomaticSyncService.ts b/src/logic/source-control/AutomaticSyncService.ts index 0f0c4dd..cb8a350 100644 --- a/src/logic/source-control/AutomaticSyncService.ts +++ b/src/logic/source-control/AutomaticSyncService.ts @@ -1,14 +1,14 @@ import type { SyncWorkspace } from '../sync/SyncWorkspace'; import { defaultSyncAction } from './ChangeActionPolicy'; import type { ChangeRepository } from './ChangeRepository'; -import type { SourceControlActionService } from './SourceControlActionService'; +import type { BackgroundSyncSession, SourceControlActionService, SyncExecutionOutcome } from './SourceControlActionService'; import type { SyncIntentRequest } from './SyncIntent'; import type { SyncChange } from './types'; export interface AutomaticSyncDependencies { workspace: Pick; changes: ChangeRepository; - actions: Pick; + actions: Pick; /** Optional diagnostic sink; automatic runs stay silent on success. */ onError?: (error: unknown) => void; } @@ -18,9 +18,17 @@ export interface AutomaticSyncDependencies { * * Reuses the existing Source Control application layer end to end: it does not * classify changes, detect renames, route actions, plan push/pull, or talk to a - * provider itself. The sequence is refresh -> read ChangeRepository -> exclude - * synced/conflict -> build default intents via ChangeActionPolicy -> execute - * through the shared Sync Queue path in background mode -> refresh again. + * provider itself. The whole transaction runs under one hold of the shared + * execution guard (via `SourceControlActionService.runBackground`), so a busy + * tick is skipped before any provider work: + * + * try-acquire guard -> refresh -> read ChangeRepository -> exclude + * synced/conflict -> default intents via ChangeActionPolicy -> (nothing to + * do: stop) -> execute in background mode -> refresh once more. + * + * Background execution never shows a user notice, so failures reported by the + * executor (rejections and per-file provider errors) are surfaced here + * through `onError`. Skipped conflicts are expected and are not errors. * * Timer mechanics deliberately live outside this service (plugin runtime * scheduling); this class only knows how to run once. @@ -32,21 +40,14 @@ export class AutomaticSyncService { /** * Runs at most one automatic sync at a time. A tick that fires while a run - * is already active is skipped rather than queued, and an execution error - * never leaves the service permanently locked. + * is already active, or while manual work owns the execution guard, is + * skipped rather than queued; an error never leaves the service locked. */ async runOnce(): Promise { if (this.running) return; this.running = true; try { - await this.dependencies.workspace.refresh(); - const intents = this.pendingIntents(); - if (intents.length > 0) { - await this.dependencies.actions.sync(intents, 'background'); - // Re-read after execution so the refreshed status reflects the - // changes that were just applied (and keeps conflicts visible). - } - await this.dependencies.workspace.refresh(); + await this.dependencies.actions.runBackground(session => this.runTransaction(session)); } catch (error) { this.dependencies.onError?.(error); } finally { @@ -54,6 +55,36 @@ export class AutomaticSyncService { } } + private async runTransaction(session: BackgroundSyncSession): Promise { + await this.dependencies.workspace.refresh(); + const intents = this.pendingIntents(); + // Idle vault (or only synced/conflict entries): the refresh above + // already published current state, so a second refresh would only + // double provider polling. + if (intents.length === 0) return; + + const outcome = await session.sync(intents); + this.reportFailures(outcome); + // Re-read after execution so the refreshed status reflects the + // changes that were just applied (and keeps conflicts visible). + await this.dependencies.workspace.refresh(); + } + + private reportFailures(outcome: SyncExecutionOutcome): void { + const { onError } = this.dependencies; + if (!onError) return; + for (const failure of outcome.failures) onError(failure); + + const result = outcome.result; + if (!result) return; + if (result.errors.length > 0) { + const details = result.errors.map(error => `${error.file}: ${error.error}`).join('; '); + onError(new Error(`Automatic sync failed for ${result.errors.length} file(s): ${details}`)); + } else if (result.failed > 0 && outcome.failures.length === 0) { + onError(new Error(`Automatic sync failed for ${result.failed} file(s)`)); + } + } + /** * Every pending change except 'synced' (nothing to do) and 'conflict' * (must be resolved manually). Actions come from the same default routing diff --git a/src/logic/source-control/SourceControlActionService.ts b/src/logic/source-control/SourceControlActionService.ts index 8513abf..dc54951 100644 --- a/src/logic/source-control/SourceControlActionService.ts +++ b/src/logic/source-control/SourceControlActionService.ts @@ -5,12 +5,23 @@ import type { OperationState } from './OperationState'; import type { SourceControlItem } from './SourceControlViewModel'; import type { SyncSelectionStore } from './SyncSelectionStore'; import { defaultSyncAction, type SyncAction } from './ChangeActionPolicy'; -import { SyncIntentExecutor, type SyncExecutionMode } from './SyncIntentExecutor'; +import { SyncIntentExecutor, type SyncExecutionMode, type SyncExecutionOutcome } from './SyncIntentExecutor'; import { SyncExecutionGuard } from './SyncExecutionGuard'; import type { SyncIntentRequest } from './SyncIntent'; import type { ChangeId, SyncChange } from './types'; export type { SyncIntentRequest } from './SyncIntent'; +export type { SyncExecutionOutcome } from './SyncIntentExecutor'; + +/** Handle given to a background transaction that already owns the execution guard. */ +export interface BackgroundSyncSession { + /** Executes intents in background mode without re-acquiring the (non-reentrant) guard. */ + sync(intents: readonly SyncIntentRequest[]): Promise; +} + +export type BackgroundRunOutcome = + | { status: 'skipped-busy' } + | { status: 'completed'; value: T }; /** Which side wins when resolving a change in the 'conflict' state. */ export type ConflictResolution = 'local' | 'remote'; @@ -172,8 +183,31 @@ export class SourceControlActionService { * SyncIntentExecutor. `mode` is per-execution: manual Sync stays * interactive, Automatic Sync passes `background`. */ - async sync(intents: readonly SyncIntentRequest[], mode: SyncExecutionMode = 'interactive'): Promise { - await this.syncIntentExecutor.execute(intents, mode); + async sync( + intents: readonly SyncIntentRequest[], + mode: SyncExecutionMode = 'interactive', + ): Promise { + return this.syncIntentExecutor.execute(intents, mode); + } + + /** + * Runs a whole background transaction (refresh -> execute -> refresh) + * under one hold of the shared guard. If the guard is busy the task is + * never invoked and nothing is queued, so a busy tick costs zero provider + * work. Because the guard hands its lock straight to the next waiter, a + * waiting manual operation still wins over a later automatic tick. + */ + async runBackground(task: (session: BackgroundSyncSession) => Promise): Promise> { + const release = this.guard.tryAcquire(); + if (!release) return { status: 'skipped-busy' }; + try { + const value = await task({ + sync: intents => this.syncIntentExecutor.executeHeld(intents, 'background'), + }); + return { status: 'completed', value }; + } finally { + release(); + } } /** Deletes one or more changes from the local vault only. */ diff --git a/src/logic/source-control/SyncIntentExecutor.ts b/src/logic/source-control/SyncIntentExecutor.ts index 77dc9f2..049b056 100644 --- a/src/logic/source-control/SyncIntentExecutor.ts +++ b/src/logic/source-control/SyncIntentExecutor.ts @@ -47,6 +47,26 @@ interface ConfirmedSyncPlan { */ export type SyncExecutionMode = 'interactive' | 'background'; +/** + * What one Sync Queue execution actually did, so callers that suppress the + * user notifier (background mode) can still observe failures. + * + * - `skipped-busy`: the shared guard was held by another mutation and a + * background run declined to queue; nothing was planned or mutated. + * - `completed`: the run finished. `result` aggregates counts and per-file + * provider errors; `failures` holds unexpected rejections from planning, + * the remote commit, or the pull apply. Skipped conflicts are neither. + */ +export interface SyncExecutionOutcome { + status: 'completed' | 'skipped-busy'; + result?: SyncExecutionResult; + failures: unknown[]; +} + +function completed(result: SyncExecutionResult = emptyExecutionResult(), failures: unknown[] = []): SyncExecutionOutcome { + return { status: 'completed', result, failures }; +} + /** * Executes the Sync Queue use-case from explicit user intent. * @@ -70,23 +90,34 @@ export class SyncIntentExecutor { private readonly guard: SyncExecutionGuard = new SyncExecutionGuard(), ) {} - async execute(intents: readonly SyncIntentRequest[], mode: SyncExecutionMode = 'interactive'): Promise { + async execute( + intents: readonly SyncIntentRequest[], + mode: SyncExecutionMode = 'interactive', + ): Promise { // Serialize automatic mutations against user-triggered ones. Automatic // work skips its tick entirely when the path is busy; manual work waits // so a user action is never discarded. const release = mode === 'background' ? this.guard.tryAcquire() : await this.guard.acquire(); - if (!release) return; + if (!release) return { status: 'skipped-busy', failures: [] }; try { - await this.executeLocked(intents, mode); + return await this.executeHeld(intents, mode); } finally { release(); } } - private async executeLocked(intents: readonly SyncIntentRequest[], mode: SyncExecutionMode): Promise { + /** + * Runs the workflow for a caller that already holds the shared guard (an + * Automatic Sync transaction spanning refresh -> execute -> refresh). The + * guard is non-reentrant, so this must never acquire it again. + */ + async executeHeld( + intents: readonly SyncIntentRequest[], + mode: SyncExecutionMode = 'background', + ): Promise { const resolved = this.resolveIntents(intents); - if (resolved.length === 0) return; + if (resolved.length === 0) return completed(); const targets = resolved.map(entry => entry.change); const buckets = this.bucket(resolved); @@ -94,13 +125,14 @@ export class SyncIntentExecutor { let plan: ConfirmedSyncPlan | null; try { plan = await this.planAndConfirm(buckets, mode); - } catch { + } catch (error) { this.failAll(targets); - if (mode !== 'background') this.notifier.notify({ ...emptyExecutionResult(), failed: targets.length }); - return; + const failedResult = { ...emptyExecutionResult(), failed: targets.length }; + if (mode !== 'background') this.notifier.notify(failedResult); + return completed(failedResult, [error]); } - if (!plan || !plan.confirmed) return; + if (!plan || !plan.confirmed) return completed(); // Paths the planner left out because they still need manual conflict // resolution. They must never be reported as successfully synced or @@ -110,12 +142,13 @@ export class SyncIntentExecutor { this.startAll(targets); const summary = emptyExecutionResult(); + const failures: unknown[] = []; if (hasRemoteMutations(plan.plannedPush, buckets.deleteRemote)) { - await this.commitRemoteBucket(plan.plannedPush, buckets.push, buckets.deleteRemote, summary, skippedPaths); + await this.commitRemoteBucket(plan.plannedPush, buckets.push, buckets.deleteRemote, summary, skippedPaths, failures); } if (buckets.pull.length > 0) { - await this.applyPullBucket(buckets.pull, summary, skippedPaths); + await this.applyPullBucket(buckets.pull, summary, skippedPaths, failures); } // Any skipped/conflicted target that a commit or pull may have marked @@ -125,6 +158,7 @@ export class SyncIntentExecutor { } if (mode !== 'background') this.notifier.notify(summary); + return completed(summary, failures); } private resolveIntents(intents: readonly SyncIntentRequest[]): ResolvedSyncIntent[] { @@ -200,6 +234,7 @@ export class SyncIntentExecutor { deleteTargets: readonly SyncChange[], summary: SyncExecutionResult, skippedPaths: ReadonlySet, + failures: unknown[], ): Promise { const targets = [...pushTargets, ...deleteTargets].filter(target => !skippedPaths.has(target.path)); try { @@ -232,9 +267,10 @@ export class SyncIntentExecutor { const failed = new Set(results.errors.map(error => error.file)); this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); addRemoteResult(summary, plannedPush, deleteEntries, results); - } catch { + } catch (error) { this.failAll(targets); summary.failed += targets.length; + failures.push(error); } } @@ -242,6 +278,7 @@ export class SyncIntentExecutor { pullTargets: readonly SyncChange[], summary: SyncExecutionResult, skippedPaths: ReadonlySet, + failures: unknown[], ): Promise { const targets = pullTargets.filter(target => !skippedPaths.has(target.path)); try { @@ -256,9 +293,10 @@ export class SyncIntentExecutor { summary.failed += results.failed; summary.conflicts += results.conflicts; summary.errors.push(...results.errors); - } catch { + } catch (error) { this.failAll(targets); summary.failed += pullTargets.length; + failures.push(error); } } diff --git a/src/ui/source-control/SourceControlItemView.ts b/src/ui/source-control/SourceControlItemView.ts index b750f7e..cd654fe 100644 --- a/src/ui/source-control/SourceControlItemView.ts +++ b/src/ui/source-control/SourceControlItemView.ts @@ -255,9 +255,9 @@ export class SourceControlItemView extends ItemView { * through the subscription above; the explicit re-render here is what * covers the failure path, where nothing else republishes status. */ - private runAction(action: Promise): Promise { + private runAction(action: Promise): Promise { this.renderView(); - return action.finally(() => this.renderView()); + return action.then(() => undefined).finally(() => this.renderView()); } /** diff --git a/tests/logic/source-control/AutomaticSyncIntegration.test.ts b/tests/logic/source-control/AutomaticSyncIntegration.test.ts new file mode 100644 index 0000000..fb10bfd --- /dev/null +++ b/tests/logic/source-control/AutomaticSyncIntegration.test.ts @@ -0,0 +1,307 @@ +import { describe, expect, it, vi, type Mock } from 'vitest'; +import { AutomaticSyncService } from '../../../src/logic/source-control/AutomaticSyncService'; +import { ChangeRepository } from '../../../src/logic/source-control/ChangeRepository'; +import { OperationState } from '../../../src/logic/source-control/OperationState'; +import { SourceControlActionService } from '../../../src/logic/source-control/SourceControlActionService'; +import { SyncSelectionStore } from '../../../src/logic/source-control/SyncSelectionStore'; +import { toChangeId, type SyncChange, type SyncChangeKind } from '../../../src/logic/source-control/types'; +import type { PlannedPushBatch } from '../../../src/logic/sync/PushCoordinator'; +import type { SyncWorkspace } from '../../../src/logic/sync/SyncWorkspace'; +import type { PushResults, SyncPlan, SyncResult } from '../../../src/logic/sync/types'; + +/** + * Real AutomaticSyncService + real SourceControlActionService/SyncIntentExecutor/ + * SyncExecutionGuard over a fake SyncWorkspace. Exercises the concurrency and + * observability contract end to end without a provider. + */ + +const change = (path: string, kind: SyncChangeKind): SyncChange => ({ id: toChangeId(path), path, kind }); + +const emptyPlan = (overrides: Partial = {}): SyncPlan => ({ additions: [], modifications: [], deletions: [], moves: [], ...overrides }); + +function plannedBatch(overrides: Partial = {}): PlannedPushBatch { + return { + reviewPlan: emptyPlan(), + pushes: [], + moves: [], + keepRemote: [], + keepLocal: [], + skippedConflicts: 0, + conflictedPaths: [], + cancelled: false, + immediate: { success: 0, updated: 0, failed: 0, errors: [], syncedPaths: [] }, + ...overrides, + }; +} + +const pushBatch = (path: string): PlannedPushBatch => plannedBatch({ + reviewPlan: emptyPlan({ modifications: [{ path, name: path }] }), + pushes: [{ path, name: path, repoPath: path, content: 'x', existingSha: 'sha' }], +}); + +const pushResults = (overrides: Partial = {}): PushResults => ({ + success: 0, added: 0, updated: 0, failed: 0, conflicts: 0, resolvedConflicts: 0, skippedConflicts: 0, errors: [], syncedPaths: [], ...overrides, +}); + +const syncResult = (overrides: Partial = {}): SyncResult => ({ success: 0, added: 0, updated: 0, failed: 0, conflicts: 0, errors: [], ...overrides }); + +function harness(changes: SyncChange[], overrides: Partial = {}) { + const repository = new ChangeRepository(); + repository.replace(changes); + const workspace = { + refresh: vi.fn().mockResolvedValue(undefined), + push: vi.fn().mockResolvedValue(pushResults()), + pull: vi.fn().mockResolvedValue(syncResult()), + toRepoPath: (path: string) => path, + planPush: vi.fn().mockResolvedValue(plannedBatch()), + planPull: vi.fn().mockResolvedValue(emptyPlan()), + applyPull: vi.fn().mockResolvedValue(syncResult()), + commitResolvedBatch: vi.fn().mockResolvedValue(undefined), + confirmPlan: vi.fn().mockResolvedValue(true), + ...overrides, + } as unknown as SyncWorkspace; + const notify = vi.fn(); + const actions = new SourceControlActionService(repository, new SyncSelectionStore(), new OperationState(), workspace, { notify }); + const onError = vi.fn(); + const automatic = new AutomaticSyncService({ workspace, changes: repository, actions, onError }); + const mocks = workspace as unknown as Record<'refresh' | 'push' | 'planPush' | 'planPull' | 'applyPull' | 'commitResolvedBatch', Mock<(...args: never[]) => Promise>>; + return { automatic, actions, notify, onError, ...mocks }; +} + +const deferred = () => { + let resolve!: () => void; + const promise = new Promise(r => { resolve = r; }); + return { promise, resolve }; +}; + +describe('Automatic Sync over the real action service', () => { + describe('busy guard', () => { + it('skips a tick entirely while manual work runs: zero refreshes, zero mutations, no queued backlog', async () => { + const manualPush = deferred(); + const h = harness( + [change('a.md', 'local-modified'), change('m.md', 'local-only')], + { push: vi.fn().mockReturnValue(manualPush.promise.then(() => pushResults())) as never }, + ); + + const manual = h.actions.push([toChangeId('m.md')]); + await vi.waitFor(() => expect(h.push).toHaveBeenCalledTimes(1)); + + await h.automatic.runOnce(); + + expect(h.refresh).not.toHaveBeenCalled(); + expect(h.planPush).not.toHaveBeenCalled(); + expect(h.commitResolvedBatch).not.toHaveBeenCalled(); + expect(h.onError).not.toHaveBeenCalled(); + + manualPush.resolve(); + await manual; + // The skipped tick did not queue: nothing runs on its own afterwards. + await Promise.resolve(); + expect(h.refresh).not.toHaveBeenCalled(); + expect(h.planPush).not.toHaveBeenCalled(); + }); + + it('runs normally on a later tick once the manual operation finished', async () => { + const manualPush = deferred(); + const h = harness( + [change('a.md', 'local-modified'), change('m.md', 'local-only')], + { push: vi.fn().mockReturnValue(manualPush.promise.then(() => pushResults())) as never }, + ); + h.planPush.mockResolvedValue(pushBatch('a.md')); + + const manual = h.actions.push([toChangeId('m.md')]); + await vi.waitFor(() => expect(h.push).toHaveBeenCalledTimes(1)); + await h.automatic.runOnce(); + manualPush.resolve(); + await manual; + + await h.automatic.runOnce(); + + expect(h.refresh).toHaveBeenCalledTimes(2); + expect(h.commitResolvedBatch).toHaveBeenCalledTimes(1); + }); + + it('still runs a manual sync that waits behind an in-flight automatic run', async () => { + const commit = deferred(); + const order: string[] = []; + const h = harness( + [change('a.md', 'local-modified'), change('m.md', 'local-only')], + { + commitResolvedBatch: vi.fn().mockImplementation(async () => { + order.push('auto-commit-start'); + await commit.promise; + order.push('auto-commit-end'); + }) as never, + push: vi.fn().mockImplementation(async () => { + order.push('manual-push'); + return pushResults(); + }) as never, + }, + ); + h.planPush.mockResolvedValue(pushBatch('a.md')); + + const auto = h.automatic.runOnce(); + await vi.waitFor(() => expect(order).toEqual(['auto-commit-start'])); + const manual = h.actions.push([toChangeId('m.md')]); + + commit.resolve(); + await auto; + await manual; + + expect(order).toEqual(['auto-commit-start', 'auto-commit-end', 'manual-push']); + }); + + it('hands the guard to a waiting manual operation ahead of a newly arriving automatic tick', async () => { + const firstPush = deferred(); + const order: string[] = []; + const h = harness( + [change('a.md', 'local-modified'), change('m.md', 'local-only'), change('n.md', 'local-only')], + { + push: vi.fn().mockImplementation(async (paths: string[]) => { + order.push(`push:${paths[0]}`); + if (paths[0] === 'm.md') await firstPush.promise; + return pushResults(); + }) as never, + }, + ); + + const first = h.actions.push([toChangeId('m.md')]); + await vi.waitFor(() => expect(order).toEqual(['push:m.md'])); + const waiting = h.actions.push([toChangeId('n.md')]); + firstPush.resolve(); + await first; + // The lock passed directly to the waiter, so this tick sees it busy. + await h.automatic.runOnce(); + await waiting; + + expect(order).toEqual(['push:m.md', 'push:n.md']); + expect(h.refresh).not.toHaveBeenCalled(); + }); + }); + + describe('refresh counts', () => { + it('refreshes exactly once for an idle vault', async () => { + const h = harness([]); + await h.automatic.runOnce(); + expect(h.refresh).toHaveBeenCalledTimes(1); + expect(h.planPush).not.toHaveBeenCalled(); + expect(h.onError).not.toHaveBeenCalled(); + }); + + it('refreshes exactly once when only synced/conflict entries exist and never resolves the conflict', async () => { + const h = harness([change('s.md', 'synced'), change('c.md', 'conflict')]); + await h.automatic.runOnce(); + expect(h.refresh).toHaveBeenCalledTimes(1); + expect(h.planPush).not.toHaveBeenCalled(); + expect(h.commitResolvedBatch).not.toHaveBeenCalled(); + expect(h.onError).not.toHaveBeenCalled(); + }); + + it('refreshes before and after a successful run and stays completely silent', async () => { + const h = harness([change('a.md', 'local-modified')]); + h.planPush.mockResolvedValue(pushBatch('a.md')); + + await h.automatic.runOnce(); + + expect(h.refresh).toHaveBeenCalledTimes(2); + expect(h.commitResolvedBatch).toHaveBeenCalledTimes(1); + expect(h.onError).not.toHaveBeenCalled(); + expect(h.notify).not.toHaveBeenCalled(); + }); + }); + + describe('failures reach the diagnostic logger, never the user notifier', () => { + it('logs a planning rejection with no mutation', async () => { + const h = harness([change('a.md', 'local-modified')]); + h.planPush.mockRejectedValue(new Error('plan exploded')); + + await h.automatic.runOnce(); + + expect(h.onError).toHaveBeenCalledWith(expect.objectContaining({ message: 'plan exploded' })); + expect(h.commitResolvedBatch).not.toHaveBeenCalled(); + expect(h.refresh).toHaveBeenCalledTimes(2); + expect(h.notify).not.toHaveBeenCalled(); + }); + + it('logs a pull planning rejection', async () => { + const h = harness([change('r.md', 'remote-only')]); + h.planPull.mockRejectedValue(new Error('pull plan exploded')); + + await h.automatic.runOnce(); + + expect(h.onError).toHaveBeenCalledWith(expect.objectContaining({ message: 'pull plan exploded' })); + expect(h.applyPull).not.toHaveBeenCalled(); + expect(h.notify).not.toHaveBeenCalled(); + }); + + it('logs a remote commit rejection after attempting the mutation', async () => { + const h = harness([change('a.md', 'local-modified')]); + h.planPush.mockResolvedValue(pushBatch('a.md')); + h.commitResolvedBatch.mockRejectedValue(new Error('commit rejected')); + + await h.automatic.runOnce(); + + expect(h.commitResolvedBatch).toHaveBeenCalledTimes(1); + expect(h.onError).toHaveBeenCalledWith(expect.objectContaining({ message: 'commit rejected' })); + expect(h.refresh).toHaveBeenCalledTimes(2); + expect(h.notify).not.toHaveBeenCalled(); + }); + + it('logs a pull apply rejection after attempting the mutation', async () => { + const h = harness([change('r.md', 'remote-only')]); + h.planPull.mockResolvedValue(emptyPlan({ additions: [{ path: 'r.md', name: 'r.md' }] })); + h.applyPull.mockRejectedValue(new Error('pull rejected')); + + await h.automatic.runOnce(); + + expect(h.applyPull).toHaveBeenCalledTimes(1); + expect(h.onError).toHaveBeenCalledWith(expect.objectContaining({ message: 'pull rejected' })); + expect(h.refresh).toHaveBeenCalledTimes(2); + expect(h.notify).not.toHaveBeenCalled(); + }); + + it('logs provider-returned per-file errors from the commit', async () => { + const h = harness([change('a.md', 'local-modified')]); + h.planPush.mockResolvedValue(pushBatch('a.md')); + const failWithProviderError = (...args: unknown[]): Promise => { + const results = args[5] as PushResults; + results.failed = 1; + results.errors.push({ file: 'a.md', error: 'HTTP 500' }); + return Promise.resolve(); + }; + h.commitResolvedBatch.mockImplementation(failWithProviderError); + + await h.automatic.runOnce(); + + expect(h.onError).toHaveBeenCalledTimes(1); + expect((h.onError.mock.calls[0]?.[0] as Error).message).toContain('a.md: HTTP 500'); + expect(h.notify).not.toHaveBeenCalled(); + }); + + it('logs provider-returned per-file errors from the pull', async () => { + const h = harness([change('r.md', 'remote-only')]); + h.planPull.mockResolvedValue(emptyPlan({ additions: [{ path: 'r.md', name: 'r.md' }] })); + h.applyPull.mockResolvedValue(syncResult({ failed: 1, errors: [{ file: 'r.md', error: 'HTTP 404' }] })); + + await h.automatic.runOnce(); + + expect(h.onError).toHaveBeenCalledTimes(1); + expect((h.onError.mock.calls[0]?.[0] as Error).message).toContain('r.md: HTTP 404'); + }); + + it('does not log skipped conflicts as errors and keeps syncing the safe paths', async () => { + const h = harness([change('safe.md', 'local-modified'), change('clash.md', 'local-modified')]); + h.planPush.mockResolvedValue(plannedBatch({ + ...pushBatch('safe.md'), + conflictedPaths: ['clash.md'], + skippedConflicts: 1, + })); + + await h.automatic.runOnce(); + + expect(h.commitResolvedBatch).toHaveBeenCalledTimes(1); + expect(h.onError).not.toHaveBeenCalled(); + expect(h.notify).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/tests/logic/source-control/AutomaticSyncService.test.ts b/tests/logic/source-control/AutomaticSyncService.test.ts index 785f48b..2bdbf55 100644 --- a/tests/logic/source-control/AutomaticSyncService.test.ts +++ b/tests/logic/source-control/AutomaticSyncService.test.ts @@ -1,7 +1,8 @@ import { describe, expect, it, vi } from 'vitest'; -import { AutomaticSyncService } from '../../../src/logic/source-control/AutomaticSyncService'; +import { AutomaticSyncService, type AutomaticSyncDependencies } from '../../../src/logic/source-control/AutomaticSyncService'; import { ChangeRepository } from '../../../src/logic/source-control/ChangeRepository'; -import type { SyncExecutionMode } from '../../../src/logic/source-control/SyncIntentExecutor'; +import type { BackgroundRunOutcome, BackgroundSyncSession } from '../../../src/logic/source-control/SourceControlActionService'; +import type { SyncExecutionOutcome } from '../../../src/logic/source-control/SyncIntentExecutor'; import type { SyncIntentRequest } from '../../../src/logic/source-control/SyncIntent'; import { toChangeId } from '../../../src/logic/source-control/types'; import type { SyncChange, SyncChangeKind } from '../../../src/logic/source-control/types'; @@ -15,19 +16,28 @@ function emptyRefreshResult(): SyncStatusRefreshResult { return {} as SyncStatusRefreshResult; } +function completedOutcome(overrides: Partial = {}): SyncExecutionOutcome { + return { status: 'completed', failures: [], ...overrides }; +} + function buildService(changes: SyncChange[], overrides: { refresh?: () => Promise; - sync?: (intents: readonly SyncIntentRequest[], mode?: SyncExecutionMode) => Promise; + sync?: (intents: readonly SyncIntentRequest[]) => Promise; + busy?: boolean; onError?: (error: unknown) => void; } = {}) { const repository = new ChangeRepository(); repository.replace(changes); const refresh = vi.fn(overrides.refresh ?? (() => Promise.resolve(emptyRefreshResult()))); - const sync = vi.fn(overrides.sync ?? (() => Promise.resolve(undefined))); + const sync = vi.fn(overrides.sync ?? (() => Promise.resolve(completedOutcome()))); + const runBackground = vi.fn(async function (task: (session: BackgroundSyncSession) => Promise): Promise> { + if (overrides.busy) return { status: 'skipped-busy' }; + return { status: 'completed', value: await task({ sync }) }; + }) as AutomaticSyncDependencies['actions']['runBackground'] & ReturnType; const service = new AutomaticSyncService({ workspace: { refresh }, changes: repository, - actions: { sync }, + actions: { runBackground }, onError: overrides.onError, }); return { service, refresh, sync, repository }; @@ -44,11 +54,13 @@ describe('AutomaticSyncService', () => { } order.push('refresh'); }); - const sync = vi.fn().mockImplementation(async () => { order.push('sync'); }); + const sync = vi.fn().mockImplementation(async () => { order.push('sync'); return completedOutcome(); }); const service = new AutomaticSyncService({ workspace: { refresh }, changes: repository, - actions: { sync }, + actions: { + runBackground: async task => ({ status: 'completed', value: await task({ sync }) }), + }, }); await service.runOnce(); @@ -86,8 +98,7 @@ describe('AutomaticSyncService', () => { await service.runOnce(); - const [intents, mode] = sync.mock.calls[0] as [SyncIntentRequest[], SyncExecutionMode]; - expect(mode).toBe('background'); + const [intents] = sync.mock.calls[0] as [SyncIntentRequest[]]; expect(intents).toEqual([ { changeId: toChangeId('push.md'), action: 'push' }, { changeId: toChangeId('pull.md'), action: 'pull' }, @@ -105,13 +116,25 @@ describe('AutomaticSyncService', () => { await service.runOnce(); expect(sync).not.toHaveBeenCalled(); - // Still refreshes before/after so the panel reflects current state. - expect(refresh).toHaveBeenCalledTimes(2); + // Exactly one refresh: nothing was executed, so a second poll would + // only double provider traffic on an idle vault. + expect(refresh).toHaveBeenCalledTimes(1); + }); + + it('performs no refresh and no sync when the execution guard is busy', async () => { + const { service, sync, refresh } = buildService([change('a.md', 'local-only')], { busy: true }); + + await service.runOnce(); + + expect(refresh).not.toHaveBeenCalled(); + expect(sync).not.toHaveBeenCalled(); }); it('does not execute concurrently on overlapping runOnce calls; the second tick is skipped', async () => { let releaseFirst: (() => void) | undefined; - const firstRun = new Promise(resolve => { releaseFirst = resolve; }); + const firstRun = new Promise(resolve => { + releaseFirst = () => resolve(completedOutcome()); + }); const { service, sync } = buildService([change('a.md', 'local-only')], { sync: () => firstRun, }); @@ -119,7 +142,6 @@ describe('AutomaticSyncService', () => { const first = service.runOnce(); const second = service.runOnce(); - // The overlapping call returns immediately without a second sync. await second; expect(sync).toHaveBeenCalledTimes(1); @@ -132,14 +154,72 @@ describe('AutomaticSyncService', () => { const onError = vi.fn(); const sync = vi.fn() .mockRejectedValueOnce(new Error('provider down')) - .mockResolvedValueOnce(undefined); + .mockResolvedValueOnce(completedOutcome()); const { service } = buildService([change('a.md', 'local-only')], { sync, onError }); await service.runOnce(); expect(onError).toHaveBeenCalledTimes(1); - // A later tick still runs and does not swallow the fresh call. await service.runOnce(); expect(sync).toHaveBeenCalledTimes(2); }); + + describe('outcome diagnostics', () => { + const zero = { added: 0, updated: 0, moved: 0, deleted: 0, downloaded: 0, acceptedRemote: 0, failed: 0, conflicts: 0, skippedConflicts: 0, errors: [] }; + + it('stays silent and refreshes twice after a successful run', async () => { + const onError = vi.fn(); + const { service, refresh } = buildService([change('a.md', 'local-only')], { + onError, + sync: async () => completedOutcome({ result: { ...zero, added: 1 } }), + }); + + await service.runOnce(); + + expect(onError).not.toHaveBeenCalled(); + expect(refresh).toHaveBeenCalledTimes(2); + }); + + it('does not treat skipped conflicts as errors', async () => { + const onError = vi.fn(); + const { service } = buildService([change('a.md', 'local-only')], { + onError, + sync: async () => completedOutcome({ result: { ...zero, skippedConflicts: 2, conflicts: 1 } }), + }); + + await service.runOnce(); + + expect(onError).not.toHaveBeenCalled(); + }); + + it('logs thrown executor failures and still refreshes afterwards', async () => { + const onError = vi.fn(); + const boom = new Error('commit rejected'); + const { service, refresh } = buildService([change('a.md', 'local-only')], { + onError, + sync: async () => completedOutcome({ result: { ...zero, failed: 1 }, failures: [boom] }), + }); + + await service.runOnce(); + + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith(boom); + expect(refresh).toHaveBeenCalledTimes(2); + }); + + it('logs provider-returned per-file errors', async () => { + const onError = vi.fn(); + const { service } = buildService([change('a.md', 'local-only')], { + onError, + sync: async () => completedOutcome({ + result: { ...zero, failed: 1, errors: [{ file: 'a.md', error: 'HTTP 500' }] }, + }), + }); + + await service.runOnce(); + + expect(onError).toHaveBeenCalledTimes(1); + expect((onError.mock.calls[0]?.[0] as Error).message).toContain('a.md: HTTP 500'); + }); + }); }); diff --git a/tests/logic/source-control/SourceControlActionService.test.ts b/tests/logic/source-control/SourceControlActionService.test.ts index 219cbc8..5deba09 100644 --- a/tests/logic/source-control/SourceControlActionService.test.ts +++ b/tests/logic/source-control/SourceControlActionService.test.ts @@ -285,7 +285,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await expect(service.sync(intents(toChangeId('c-1')))).resolves.toBeUndefined(); + await expect(service.sync(intents(toChangeId('c-1')))).resolves.toMatchObject({ status: 'completed', failures: [expect.any(Error)] }); expect(operations.get(toChangeId('c-1'))).toBe('failed'); expect(notify).toHaveBeenCalledTimes(1); @@ -305,7 +305,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await expect(service.sync(intents(toChangeId('c-1')))).resolves.toBeUndefined(); + await expect(service.sync(intents(toChangeId('c-1')))).resolves.toMatchObject({ status: 'completed', failures: [expect.any(Error)] }); expect(operations.get(toChangeId('c-1'))).toBe('failed'); expect(notify).toHaveBeenCalledTimes(1); diff --git a/tests/runtime/createSyncRuntime.test.ts b/tests/runtime/createSyncRuntime.test.ts index 6ee05e9..1d899f4 100644 --- a/tests/runtime/createSyncRuntime.test.ts +++ b/tests/runtime/createSyncRuntime.test.ts @@ -71,17 +71,17 @@ describe('createSyncRuntime', () => { statuses: new Map(), remoteEntries: [], } as never); - const sync = vi.spyOn(runtime.sourceControlActions, 'sync').mockResolvedValue(undefined); + const sync = vi.fn().mockResolvedValue({ status: 'completed', failures: [] }); + const runBackground = vi.spyOn(runtime.sourceControlActions, 'runBackground') + .mockImplementation(task => task({ sync }).then(value => ({ status: 'completed' as const, value }))); runtime.sync.status.set({ path: 'note.md', status: 'unsynced' }); await runtime.automaticSync.runOnce(); expect(refresh).toHaveBeenCalledTimes(2); + expect(runBackground).toHaveBeenCalledTimes(1); expect(sync).toHaveBeenCalledTimes(1); - expect(sync).toHaveBeenCalledWith( - [expect.objectContaining({ changeId: 'note.md' })], - 'background', - ); + expect(sync).toHaveBeenCalledWith([expect.objectContaining({ changeId: 'note.md' })]); }); it('keeps ChangeRepository in sync with the shared SyncStatusService until disposed', () => { From 409e800c81e7b19820524eebc3ab13bf7bd269b2 Mon Sep 17 00:00:00 2001 From: tianyao Date: Mon, 21 Sep 2026 04:55:47 +0000 Subject: [PATCH 4/7] fix(sync): serialize legacy manual provider mutations through the shared execution guard Ribbon/command/context-menu push and pull and Push/Pull All called SyncManager directly, bypassing SyncExecutionGuard, so they could overlap Automatic Sync. Add SourceControlActionService.runManual (same single guard, non-reentrant) and route those entry points through it; Push/Pull All re-read the remote tree inside the guard. Also make e2e cleanup tolerate an unset E2E_TEST_BRANCH. Co-Authored-By: Claude Sonnet 5 --- feature_list.json | 6 +- progress.md | 6 +- scripts/e2e-harness.sh | 13 +- session-handoff.md | 11 +- .../SourceControlActionService.ts | 14 ++ src/main.ts | 45 ++++-- .../ManualSerialization.test.ts | 139 ++++++++++++++++++ 7 files changed, 211 insertions(+), 23 deletions(-) create mode 100644 tests/logic/source-control/ManualSerialization.test.ts diff --git a/feature_list.json b/feature_list.json index b5879a3..d33b246 100644 --- a/feature_list.json +++ b/feature_list.json @@ -1,14 +1,14 @@ { "_note": "GitHub Issues (firstsun-dev/git-files-sync, Project #6) is the source of truth for the full backlog and priority/estimate fields. This file mirrors only the active feature and the next few candidates so an agent session has a local, offline checkpoint — sync it against `gh issue list --repo firstsun-dev/git-files-sync --state open` at the start of a session rather than treating it as authoritative.", - "_lastSync": "2026-09-12: Issue #141 (Automatic Syncing) implemented on claude/automatic-sync-141; #143/#139 entries below are carried over from the base branch history.", + "_lastSync": "2026-09-12: Issue #141 (Automatic Syncing) merged into claude/mobile-source-control-density (combined PR #156); #143/#139 entries below are carried over from the base branch history.", "features": [ { "id": "feat-029", "name": "feat(sync): automatic scheduled sync (issue #141)", "description": "Scheduled + optional startup automatic sync that reuses the Source Control execution path in a background policy, skipping conflicts safely.", - "dependencies": ["claude/mobile-source-control-density (PR #156)"], + "dependencies": [], "status": "in-review", - "evidence": "Branch claude/automatic-sync-141; eslint 0 errors, build passed, vitest 80 files / 1010 tests passed; stacked PR pending." + "evidence": "Combined PR #156 on claude/mobile-source-control-density; eslint 0 errors, build passed, vitest 82 files / 1035 tests passed; all user-triggered provider mutations share the one SyncExecutionGuard." }, { "id": "feat-027", diff --git a/progress.md b/progress.md index fee3e98..2123aba 100644 --- a/progress.md +++ b/progress.md @@ -23,12 +23,16 @@ Below that: the previous "Outstanding Items"/"Verification Evidence" entries tra ## Verification Evidence +2026-09-21 review fixes (manual-mutation serialization via `SourceControlActionService.runManual`, e2e cleanup unbound-var fix): + +- `npx eslint .` — 0 errors. `npm run build` — passed. `npx vitest run` — 82 files / 1035 tests passed (new: `tests/logic/source-control/ManualSerialization.test.ts`). + 2026-09-21 review fixes (Automatic Sync observability / busy skip / single refresh): - `npx eslint .` — 0 errors. `npm run build` (tsc + Obsidian 1.11.0 compat + esbuild) — passed. `npx vitest run` — 81 files / 1030 tests passed (new: `tests/logic/source-control/AutomaticSyncIntegration.test.ts`, 14 tests over the real action service). - Provider E2E (`npm run test:e2e`) **not run**: no provider credentials in this environment. -This session (Issue #141 — Automatic Syncing, `claude/automatic-sync-141` stacked on `claude/mobile-source-control-density`): +Earlier session (Issue #141 — Automatic Syncing; since merged into `claude/mobile-source-control-density` / combined PR #156 via #159): - `npx eslint .` — 0 errors, 0 warnings. - `npm run build` (tsc + Obsidian 1.11.0 compat typecheck + esbuild) — passed. diff --git a/scripts/e2e-harness.sh b/scripts/e2e-harness.sh index 33ab777..796283c 100755 --- a/scripts/e2e-harness.sh +++ b/scripts/e2e-harness.sh @@ -250,12 +250,23 @@ cmd_cleanup() { return fi load_env_file - setup_askpass + # If provision failed before assigning the branch (e.g. a network error), + # there is nothing to delete. Never let cleanup throw (`set -u`) and mask + # the original failure. + if [ -z "${E2E_TEST_BRANCH:-}" ]; then + log "No E2E_TEST_BRANCH recorded (provisioning did not get that far) — nothing to clean up" + return + fi if [ "$keep_branch" = "1" ]; then log "E2E_KEEP_BRANCH set — leaving $E2E_TEST_BRANCH in place" return fi local dir; dir=$(clone_dir) + if [ ! -d "$dir/.git" ]; then + log "No clone at $dir — remote branch was never created by this run" + return + fi + setup_askpass log "Deleting isolated branch $E2E_TEST_BRANCH" git_network -C "$dir" push origin ":refs/heads/${E2E_TEST_BRANCH}" || true } diff --git a/session-handoff.md b/session-handoff.md index 095d59a..e627d64 100644 --- a/session-handoff.md +++ b/session-handoff.md @@ -1,8 +1,8 @@ # Session Handoff **Date:** 2026-09-12 -**Active feature:** Issue #141 — Automatic Syncing (target v1.7.0). Implementation complete and locally verified; PR pending. -**Branch:** `claude/automatic-sync-141`, a child branch of `origin/claude/mobile-source-control-density` (`a1f01b3`, PR #156 still open). The #141 PR must be stacked with base `claude/mobile-source-control-density`; retarget the SAME PR to `main` only if #156 merges first. Do not commit #141 work onto #156. +**Active feature:** Issue #141 — Automatic Syncing (target v1.7.0), combined with mobile Source Control density in PR #156. Implementation complete; review fixes applied. +**Branch:** `claude/mobile-source-control-density` — combined PR #156 (mobile density + Automatic Sync, already merged in via #159). Do not split them again. ## Completed this session @@ -21,14 +21,13 @@ - `npx eslint .` — 0 errors, 0 warnings. - `npm run build` — passed (tsc + Obsidian 1.11.0 compat typecheck + esbuild). -- `npx vitest run` — 80 files / 1010 tests passed. +- `npx vitest run` — 82 files / 1035 tests passed (as of the manual-serialization review fix; re-check after merging main). - NOT done: manual Obsidian runtime verification (no executable Obsidian here) and real-provider E2E. Both are explicitly noted in the PR body; do not claim they ran. ## Next steps -1. Commit and push `claude/automatic-sync-141`; open the stacked PR (base `claude/mobile-source-control-density`) titled `feat(sync): add automatic scheduled sync`, body includes `Closes #141` + the manual checklist. -2. If #156 merges: merge latest `main` into this SAME branch and retarget the SAME PR to `main`. -3. Manual Obsidian verification (desktop + mobile) against the checklist in the PR. +1. Merge PR #156 (title: `feat(sync): add automatic sync and refine mobile Source Control density`) once Required Checks are green. +2. Manual Obsidian verification (desktop + mobile) against the checklist in the PR. 4. Do not hand-bump `manifest.json`/`package.json`/`versions.json`/generated `CHANGELOG.md`; semantic-release performs the 1.7.0 bump. ## Gotchas diff --git a/src/logic/source-control/SourceControlActionService.ts b/src/logic/source-control/SourceControlActionService.ts index dc54951..bd6b74f 100644 --- a/src/logic/source-control/SourceControlActionService.ts +++ b/src/logic/source-control/SourceControlActionService.ts @@ -84,6 +84,20 @@ export class SourceControlActionService { } } + /** + * Serialization boundary for legacy/manual entry points that still call + * SyncManager directly (ribbon, commands, file context menu, Push/Pull All). + * Uses the SAME guard as every other operation here (manual semantics: + * waits, never discards). + * + * The guard is non-reentrant: never wrap sync()/push()/pull()/deleteRemote()/ + * deleteLocal()/resolveConflict()/runBackground() in this — they already + * acquire it, so doing so would deadlock. + */ + async runManual(operation: () => Promise): Promise { + return this.serialized(operation); + } + /** Adds one change to the Sync Queue. */ selectForSync(changeId: ChangeId): void { this.selection.selectForSync(changeId); diff --git a/src/main.ts b/src/main.ts index 2625a03..92b9551 100644 --- a/src/main.ts +++ b/src/main.ts @@ -137,7 +137,7 @@ export default class GitLabFilesPush extends Plugin { this.pushRibbonEl = this.addRibbonIcon('upload-cloud', this.pushRibbonLabel(), async () => { const activeView = this.app.workspace.getActiveViewOfType(MarkdownView); if (activeView && activeView.file instanceof TFile) { - await this.sync.pushFiles([activeView.file]); + await this.pushFileSerialized(activeView.file); } else { new Notice(t('main.notice.noActiveNote')); } @@ -153,7 +153,7 @@ export default class GitLabFilesPush extends Plugin { callback: async () => { const activeView = this.app.workspace.getActiveViewOfType(MarkdownView); if (activeView && activeView.file instanceof TFile) { - await this.sync.pushFiles([activeView.file]); + await this.pushFileSerialized(activeView.file); } } }); @@ -164,7 +164,7 @@ export default class GitLabFilesPush extends Plugin { callback: async () => { const activeView = this.app.workspace.getActiveViewOfType(MarkdownView); if (activeView && activeView.file instanceof TFile) { - await this.sync.pullFile(activeView.file); + await this.pullFileSerialized(activeView.file); } } }); @@ -191,12 +191,12 @@ export default class GitLabFilesPush extends Plugin { menu.addItem((item) => { item.setTitle(t('main.contextMenu.pushTo', { service: this.serviceName })) .setIcon('upload-cloud') - .onClick(async () => { await this.sync.pushFiles([file]); }); + .onClick(async () => { await this.pushFileSerialized(file); }); }); menu.addItem((item) => { item.setTitle(t('main.contextMenu.pullFrom', { service: this.serviceName })) .setIcon('download-cloud') - .onClick(async () => { await this.sync.pullFile(file); }); + .onClick(async () => { await this.pullFileSerialized(file); }); }); } }) @@ -540,6 +540,15 @@ export default class GitLabFilesPush extends Plugin { ?.getPath() ?? null; } + /** Single-file push/pull enter the shared execution guard so they never overlap Automatic Sync. */ + private async pushFileSerialized(file: TFile): Promise { + await this.sourceControlActions.runManual(() => this.sync.pushFiles([file])); + } + + private async pullFileSerialized(file: TFile): Promise { + await this.sourceControlActions.runManual(() => this.sync.pullFile(file)); + } + async pushAllFiles(): Promise { await this.runAllFiles('push'); } @@ -593,13 +602,25 @@ export default class GitLabFilesPush extends Plugin { const progressNotice = new Notice(t('main.progress.running', { verb: runVerb, total: files.length }), 0); try { - const results = op === 'push' - ? await this.sync.pushFiles(files, (current, total, fileName) => { - progressNotice.setMessage(t('main.progress.step', { verb: t('main.verb.pushing'), current, total, fileName })); - }, tree) - : await this.sync.pullAllFiles(files, (current, total, fileName) => { - progressNotice.setMessage(t('main.progress.step', { verb: t('main.verb.pulling'), current, total, fileName })); - }, tree); + // The guard is taken only after the user confirms (never held during the + // dialog). The remote tree decides the mutation plan, so it is re-read + // inside the guard: the pre-confirm `tree` above is only for gitignore + // discovery and may be stale if Automatic Sync committed meanwhile. + const results = await this.sourceControlActions.runManual(async () => { + let authoritativeTree: GitTreeEntry[] | undefined; + try { + authoritativeTree = await this.gitService.listFilesDetailed(this.settings.branch, false); + } catch (e) { + logger.warn('Failed to re-fetch remote tree under guard; falling back to per-call fetches', e); + } + return op === 'push' + ? await this.sync.pushFiles(files, (current, total, fileName) => { + progressNotice.setMessage(t('main.progress.step', { verb: t('main.verb.pushing'), current, total, fileName })); + }, authoritativeTree) + : await this.sync.pullAllFiles(files, (current, total, fileName) => { + progressNotice.setMessage(t('main.progress.step', { verb: t('main.verb.pulling'), current, total, fileName })); + }, authoritativeTree); + }); progressNotice.hide(); diff --git a/tests/logic/source-control/ManualSerialization.test.ts b/tests/logic/source-control/ManualSerialization.test.ts new file mode 100644 index 0000000..15b707b --- /dev/null +++ b/tests/logic/source-control/ManualSerialization.test.ts @@ -0,0 +1,139 @@ +import { describe, expect, it, vi } from 'vitest'; +import { AutomaticSyncService } from '../../../src/logic/source-control/AutomaticSyncService'; +import { ChangeRepository } from '../../../src/logic/source-control/ChangeRepository'; +import { OperationState } from '../../../src/logic/source-control/OperationState'; +import { SourceControlActionService } from '../../../src/logic/source-control/SourceControlActionService'; +import { SyncSelectionStore } from '../../../src/logic/source-control/SyncSelectionStore'; +import { toChangeId, type SyncChange, type SyncChangeKind } from '../../../src/logic/source-control/types'; +import type { SyncWorkspace } from '../../../src/logic/sync/SyncWorkspace'; +import type { PushResults, SyncPlan, SyncResult } from '../../../src/logic/sync/types'; + +/** + * runManual() is the serialization boundary for legacy entry points (ribbon, + * commands, context menu, Push/Pull All) that call SyncManager directly. It + * must share the one real guard with Automatic Sync and the action service. + */ + +const change = (path: string, kind: SyncChangeKind): SyncChange => ({ id: toChangeId(path), path, kind }); +const emptyPlan = (): SyncPlan => ({ additions: [], modifications: [], deletions: [], moves: [] }); +const pushResults = (): PushResults => ({ + success: 0, added: 0, updated: 0, failed: 0, conflicts: 0, resolvedConflicts: 0, skippedConflicts: 0, errors: [], syncedPaths: [], +}); +const syncResult = (): SyncResult => ({ success: 0, added: 0, updated: 0, failed: 0, conflicts: 0, errors: [] }); + +const deferred = () => { + let resolve!: () => void; + const promise = new Promise(r => { resolve = r; }); + return { promise, resolve }; +}; + +function harness(changes: SyncChange[], overrides: Partial = {}) { + const repository = new ChangeRepository(); + repository.replace(changes); + const workspace = { + refresh: vi.fn().mockResolvedValue(undefined), + push: vi.fn().mockResolvedValue(pushResults()), + pull: vi.fn().mockResolvedValue(syncResult()), + toRepoPath: (path: string) => path, + planPush: vi.fn().mockResolvedValue({ + reviewPlan: emptyPlan(), pushes: [], moves: [], keepRemote: [], keepLocal: [], skippedConflicts: 0, + conflictedPaths: [], cancelled: false, immediate: { success: 0, updated: 0, failed: 0, errors: [], syncedPaths: [] }, + }), + planPull: vi.fn().mockResolvedValue(emptyPlan()), + applyPull: vi.fn().mockResolvedValue(syncResult()), + commitResolvedBatch: vi.fn().mockResolvedValue(undefined), + confirmPlan: vi.fn().mockResolvedValue(true), + ...overrides, + } as unknown as SyncWorkspace; + const actions = new SourceControlActionService(repository, new SyncSelectionStore(), new OperationState(), workspace); + const automatic = new AutomaticSyncService({ workspace, changes: repository, actions, onError: vi.fn() }); + return { actions, automatic, workspace: workspace as unknown as Record<'refresh' | 'planPush' | 'commitResolvedBatch', ReturnType> }; +} + +describe('SourceControlActionService.runManual', () => { + it('blocks a direct manual mutation until an in-flight automatic run finishes', async () => { + const commit = deferred(); + const order: string[] = []; + const h = harness([change('a.md', 'local-modified')], { + commitResolvedBatch: vi.fn().mockImplementation(async () => { + order.push('automatic-start'); + await commit.promise; + order.push('automatic-end'); + }) as never, + }); + h.workspace.planPush.mockResolvedValue({ + reviewPlan: { ...emptyPlan(), modifications: [{ path: 'a.md', name: 'a.md' }] }, + pushes: [{ path: 'a.md', name: 'a.md', repoPath: 'a.md', content: 'x', existingSha: 's' }], + moves: [], keepRemote: [], keepLocal: [], skippedConflicts: 0, conflictedPaths: [], cancelled: false, + immediate: { success: 0, updated: 0, failed: 0, errors: [], syncedPaths: [] }, + }); + + const auto = h.automatic.runOnce(); + await vi.waitFor(() => expect(order).toEqual(['automatic-start'])); + + const manual = h.actions.runManual(async () => { + order.push('manual-start'); + order.push('manual-end'); + }); + await Promise.resolve(); + await Promise.resolve(); + expect(order).toEqual(['automatic-start']); + + commit.resolve(); + await auto; + await manual; + expect(order).toEqual(['automatic-start', 'automatic-end', 'manual-start', 'manual-end']); + }); + + it('makes an automatic tick skip with zero refresh/plan/mutation, without queueing it', async () => { + const manualDone = deferred(); + const h = harness([change('a.md', 'local-modified')]); + + const manual = h.actions.runManual(() => manualDone.promise); + await h.automatic.runOnce(); + + expect(h.workspace.refresh).not.toHaveBeenCalled(); + expect(h.workspace.planPush).not.toHaveBeenCalled(); + expect(h.workspace.commitResolvedBatch).not.toHaveBeenCalled(); + + manualDone.resolve(); + await manual; + await Promise.resolve(); + expect(h.workspace.refresh).not.toHaveBeenCalled(); + + await h.automatic.runOnce(); + expect(h.workspace.refresh).toHaveBeenCalled(); + }); + + it('keeps fairness: a waiting manual operation gets the lock ahead of a fresh automatic tick', async () => { + const aDone = deferred(); + const order: string[] = []; + const h = harness([change('a.md', 'local-modified')]); + + const a = h.actions.runManual(async () => { order.push('A'); await aDone.promise; }); + const b = h.actions.runManual(async () => { order.push('B'); }); + aDone.resolve(); + await a; + await h.automatic.runOnce(); + await b; + + expect(order).toEqual(['A', 'B']); + expect(h.workspace.refresh).not.toHaveBeenCalled(); + }); + + it('does not double-acquire: push/pull/sync on the action service still complete', async () => { + const h = harness([change('a.md', 'local-modified'), change('b.md', 'remote-modified')]); + + await expect(h.actions.push([toChangeId('a.md')])).resolves.toBeUndefined(); + await expect(h.actions.pull([toChangeId('b.md')])).resolves.toBeUndefined(); + await expect(h.actions.sync([])).resolves.toBeDefined(); + // Guard is released after each: a subsequent runManual is not stuck. + await expect(h.actions.runManual(async () => 'ok')).resolves.toBe('ok'); + }); + + it('releases the guard when the manual operation throws', async () => { + const h = harness([]); + await expect(h.actions.runManual(() => Promise.reject(new Error('boom')))).rejects.toThrow('boom'); + await expect(h.actions.runManual(async () => 'next')).resolves.toBe('next'); + }); +}); From 12c213d3025e9153d5b7317d2cf09b2c1c6e9bf5 Mon Sep 17 00:00:00 2001 From: tianyao Date: Mon, 21 Sep 2026 04:56:25 +0000 Subject: [PATCH 5/7] docs: fix handoff step numbering Co-Authored-By: Claude Sonnet 5 --- session-handoff.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/session-handoff.md b/session-handoff.md index e627d64..def9bd8 100644 --- a/session-handoff.md +++ b/session-handoff.md @@ -28,7 +28,7 @@ 1. Merge PR #156 (title: `feat(sync): add automatic sync and refine mobile Source Control density`) once Required Checks are green. 2. Manual Obsidian verification (desktop + mobile) against the checklist in the PR. -4. Do not hand-bump `manifest.json`/`package.json`/`versions.json`/generated `CHANGELOG.md`; semantic-release performs the 1.7.0 bump. +3. Do not hand-bump `manifest.json`/`package.json`/`versions.json`/generated `CHANGELOG.md`; semantic-release performs the 1.7.0 bump. ## Gotchas From 243196ca44c4bae390367ec56848846d1ffbb160 Mon Sep 17 00:00:00 2001 From: tianyao Date: Mon, 21 Sep 2026 07:28:49 +0000 Subject: [PATCH 6/7] fix(e2e): make harness cleanup network-free when no run state was persisted cmd_cleanup called load_env_file (-> normalize_env -> GitLab curl) before checking for a recorded branch, so a provisioning network failure could be repeated and masked by cleanup. Return early when e2e.env is absent and no branch is inherited. Refresh stale progress.md / feature_list.json status. Co-Authored-By: Claude Sonnet 5 --- feature_list.json | 2 +- progress.md | 13 +++++++++---- scripts/e2e-harness.sh | 13 ++++++++++--- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/feature_list.json b/feature_list.json index d33b246..3c634dd 100644 --- a/feature_list.json +++ b/feature_list.json @@ -1,6 +1,6 @@ { "_note": "GitHub Issues (firstsun-dev/git-files-sync, Project #6) is the source of truth for the full backlog and priority/estimate fields. This file mirrors only the active feature and the next few candidates so an agent session has a local, offline checkpoint — sync it against `gh issue list --repo firstsun-dev/git-files-sync --state open` at the start of a session rather than treating it as authoritative.", - "_lastSync": "2026-09-12: Issue #141 (Automatic Syncing) merged into claude/mobile-source-control-density (combined PR #156); #143/#139 entries below are carried over from the base branch history.", + "_lastSync": "2026-09-21: Issue #141 (Automatic Syncing) merged into claude/mobile-source-control-density (combined PR #156); #143/#139 entries below are carried over from the base branch history.", "features": [ { "id": "feat-029", diff --git a/progress.md b/progress.md index 2123aba..a25c628 100644 --- a/progress.md +++ b/progress.md @@ -5,24 +5,29 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont ## Current State **Last Updated:** 2026-09-21 -**Active Feature:** Issue #141 — Automatic Syncing (v1.7.0). Implementation complete, verified locally; PR pending. +**Active Feature:** Issue #141 — Automatic Syncing (v1.7.0). Implementation complete; PR #156 reviewed, CI green at `12c213d` (run 35562916807). **Branch / PR:** `claude/mobile-source-control-density` / PR #156. #156 is now a combined PR: Mobile Source Control density (CSS + structural tests) **and** Automatic Sync (#141, merged in via #159). It is no longer CSS-only. semantic-release owns the 1.7.0 bump. **What landed (#141):** persisted `automaticSyncEnabled` / `automaticSyncIntervalMinutes` / `automaticSyncOnStartup` (defaults OFF / 5 / OFF, interval min 1); settings UI rows distinct from the existing `autoRefreshOnStartup`; EN/zh-TW/zh-CN strings; `AutomaticSyncService` (refresh → repository → default intents → background execute → refresh) wired through `createSyncRuntime`; `AutomaticSyncScheduler` in plugin runtime; `SyncExecutionMode` per-execution policy with `PushConflictBehavior = 'skip'` at the `PushCoordinator` planning boundary; `SyncExecutionGuard` serialization; startup sync that never opens Source Control and supersedes the legacy startup refresh; hand-curated 1.7.0 What's New entry. **Review fixes (2026-09-21):** background failures now reach `onError` via `SyncExecutionOutcome`; a busy tick is skipped before any refresh/planning via `SourceControlActionService.runBackground` (one shared `SyncExecutionGuard`, held across refresh → execute → refresh); the redundant second refresh on an idle vault is gone (idle/synced+conflict-only = 1 refresh, executed run = 2, busy = 0). -**Next:** monitor CI on #156. Manual Obsidian verification not performed in this environment (no executable Obsidian runtime) — checklist is in the PR body. +**Next:** merge #156 once CI is green on the final e2e-cleanup fix head. Manual Obsidian verification not performed in this environment (no executable Obsidian runtime) — checklist is in the PR body. 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. ## Outstanding Items -1. Run `npm run test:e2e -- --provider github`, `gitlab`, and `gitea` with provisioned credentials; verify mixed-100 remains under 120s (target <30s) and the provider matrix passes. -2. Commit and push the current working tree, then monitor the CI provider matrix. +1. Confirm CI is green on the head containing the e2e-harness cleanup fix (no persisted run state ⇒ network-free cleanup), then merge #156. +2. Manual Obsidian runtime verification (checklist in the PR body) — no executable Obsidian in this environment. ## Verification Evidence +2026-09-21 e2e cleanup fix (`scripts/e2e-harness.sh cleanup` is network-free when provisioning never wrote `e2e.env`; previously `load_env_file` → `normalize_env` could repeat the GitLab `curl` and mask the original failure): + +- Fake-`curl`/`git`/`docker` shell check: gitlab + empty workdir → exit 0, 0 curl/fetch/push calls; state file without branch → exit 0, nothing deleted; github state + branch + clone → reaches `git push origin :refs/heads/`; gitea cleanup unchanged. +- Provider E2E (GitHub/GitLab/Gitea) + Required Checks green on `12c213d` (run 35562916807; 82 files / 1035 tests, Node 22 + 24). + 2026-09-21 review fixes (manual-mutation serialization via `SourceControlActionService.runManual`, e2e cleanup unbound-var fix): - `npx eslint .` — 0 errors. `npm run build` — passed. `npx vitest run` — 82 files / 1035 tests passed (new: `tests/logic/source-control/ManualSerialization.test.ts`). diff --git a/scripts/e2e-harness.sh b/scripts/e2e-harness.sh index 796283c..e617e1e 100755 --- a/scripts/e2e-harness.sh +++ b/scripts/e2e-harness.sh @@ -249,10 +249,17 @@ cmd_cleanup() { rm -f "$workdir/e2e.env" "$workdir/e2e.secrets.env" return fi + # write_env_file runs only after provision assigned the branch. Without + # that file (and no branch inherited from the caller) this run created + # nothing, and load_env_file would re-run normalize_env, whose provider + # discovery may hit the network again -- repeating (and masking) the + # original provisioning failure. Bail out before any of it. + if [ ! -f "$workdir/e2e.env" ] && [ -z "${E2E_TEST_BRANCH:-}" ]; then + log "No persisted E2E run state at $workdir/e2e.env — nothing to clean up" + return + fi load_env_file - # If provision failed before assigning the branch (e.g. a network error), - # there is nothing to delete. Never let cleanup throw (`set -u`) and mask - # the original failure. + # Never let cleanup throw (`set -u`) on a state file lacking the branch. if [ -z "${E2E_TEST_BRANCH:-}" ]; then log "No E2E_TEST_BRANCH recorded (provisioning did not get that far) — nothing to clean up" return From 753b840ae2163fad139ae7b0bb7e1d7980e69b8f Mon Sep 17 00:00:00 2001 From: tianyao Date: Mon, 21 Sep 2026 10:20:19 +0000 Subject: [PATCH 7/7] chore(ci): migrate actions to Node 24 runtime Tracks firstsun-dev/.github#29 Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7617202..252aa58 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -78,7 +78,7 @@ jobs: e2e-relevant: ${{ steps.filter.outputs.e2e-relevant }} steps: - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - - uses: dorny/paths-filter@15192bc058cc28a13dbf6cde61f19e18988b7af6 # v3 + - uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4 id: filter with: filters: |