From fd6d7ec4798830e27119594701dc493b9ecbf438 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 00:40:43 +0000 Subject: [PATCH 01/26] fix(source-control): dedupe mobile diff render, force unified on phones renderDiffViewer rendered an empty diff panel synchronously, then SourceControlView's async load appended a second, real one alongside it via a raw renderDiffPanel(container, ...) call -- two Remote/Local panels stacked on screen, the top one permanently empty. DiffViewer now owns the diff body's whole lifecycle: content-less options render just the body, and callers fill it in later through a returned handle's setContent(), which empties the body before rendering so there's ever only one panel. Also stops relying on Platform.isMobile alone for split/unified: tablets report isMobile too, so the previous "mobile defaults to unified" policy didn't stop a user's saved split preference from producing unreadable ~150px columns on an actual phone. renderDiffViewer now forces unified and hides the layout toggle whenever Platform.isPhone is true, regardless of session preference, across all three diff surfaces (diff tab, conflict modal, mobile detail) that share it. Adds defensive min-width/max-width and minmax(0, 1fr) grid tracks so a long unbroken diff line can't push the split panel past its container. --- src/ui/components/DiffViewer.ts | 64 +++++++++++---- src/ui/source-control/SourceControlView.ts | 22 +++--- styles.css | 10 ++- tests/setup.ts | 2 +- tests/ui/components/DiffViewer.test.ts | 52 +++++++++++- .../source-control/SourceControlView.test.ts | 79 ++++++++++++++++++- 6 files changed, 196 insertions(+), 33 deletions(-) diff --git a/src/ui/components/DiffViewer.ts b/src/ui/components/DiffViewer.ts index 1d8e5f4..1f5587f 100644 --- a/src/ui/components/DiffViewer.ts +++ b/src/ui/components/DiffViewer.ts @@ -35,43 +35,75 @@ export function resetDiffLayoutMemoryForTests(): void { } export interface DiffViewerOptions { - remote: string; - local: string; + /** + * Diff content. Omit both when the content isn't loaded yet (e.g. an + * async fetch is in flight) — the viewer renders just the empty body, + * and the caller fills it in later via the returned handle's + * `setContent`. Passing only one of the two is not supported. + */ + remote?: string; + local?: string; layout: DiffLayout; /** * Where the split/unified toggle renders — typically the fixed header * region of the enclosing surface, so it stays reachable while a long * diff scrolls below. When omitted, no toggle is rendered and the viewer - * shows the given layout statically. + * shows the given layout statically. Also suppressed on phones (see + * `renderDiffViewer` doc) regardless of this option. */ toggleHost?: HTMLElement; /** State-sync callback so the owning surface can persist the layout across its own re-renders. */ onLayoutChange?: (next: DiffLayout) => void; } +/** Handle to the diff body a `renderDiffViewer` call created, for filling in content that wasn't ready yet at render time. */ +export interface DiffViewerHandle { + /** Replaces the body's content. Safe to call once the initial (possibly content-less) render has happened. */ + setContent(remote: string, local: string): void; +} + /** * The shared diff-viewer composition: layout toggle + body layout class + * diff panel. Every diff surface (desktop diff tab, conflict modal, mobile * detail) renders through this instead of reassembling DiffLayoutToggle and * DiffPanel — and never rebuilds its own "apply layout class + re-render - * toggle" dance. + * toggle" dance. This is also the single owner of the diff body element: + * callers that load content asynchronously must go through the returned + * handle's `setContent` rather than reaching into the DOM and calling + * `renderDiffPanel` themselves, which would append a second copy alongside + * whatever this function already rendered. + * + * Phones (`Platform.isPhone`) always render unified with no toggle — a + * split view is unreadable at phone width, and offering a toggle that + * produces two ~150px columns is worse than not offering it. Tablets and + * desktop keep the caller's requested layout and toggle. */ -export function renderDiffViewer(container: HTMLElement, options: DiffViewerOptions): void { - const body = container.createDiv({ cls: `scv-diff-tab-body scv-diff-layout-${options.layout}` }); - renderDiffPanel(body, options.remote, options.local); +export function renderDiffViewer(container: HTMLElement, options: DiffViewerOptions): DiffViewerHandle { + const layout: DiffLayout = Platform.isPhone ? 'unified' : options.layout; + const body = container.createDiv({ cls: `scv-diff-tab-body scv-diff-layout-${layout}` }); + if (options.remote !== undefined && options.local !== undefined) { + renderDiffPanel(body, options.remote, options.local); + } const toggleHost = options.toggleHost; - if (!toggleHost) return; + if (toggleHost && !Platform.isPhone) { + const renderToggle = (l: DiffLayout): void => { + toggleHost.empty(); + renderDiffLayoutToggle(toggleHost, l, next => { + applyLayoutClass(body, next); + options.onLayoutChange?.(next); + renderToggle(next); + }); + }; + renderToggle(layout); + } - const renderToggle = (layout: DiffLayout): void => { - toggleHost.empty(); - renderDiffLayoutToggle(toggleHost, layout, next => { - applyLayoutClass(body, next); - options.onLayoutChange?.(next); - renderToggle(next); - }); + return { + setContent(remote: string, local: string): void { + body.empty(); + renderDiffPanel(body, remote, local); + }, }; - renderToggle(options.layout); } function applyLayoutClass(body: HTMLElement, layout: DiffLayout): void { diff --git a/src/ui/source-control/SourceControlView.ts b/src/ui/source-control/SourceControlView.ts index fffc04b..5a6de09 100644 --- a/src/ui/source-control/SourceControlView.ts +++ b/src/ui/source-control/SourceControlView.ts @@ -5,8 +5,7 @@ import { SourceControlViewModel, type SourceControlItem } from '../../logic/sour import type { ChangeId } from '../../logic/source-control/types'; import { defaultSyncAction } from '../../logic/source-control/ChangeActionPolicy'; import { ICONS } from '../components/icons'; -import { renderDiffViewer, currentDiffLayout, rememberDiffLayout } from '../components/DiffViewer'; -import { renderDiffPanel } from '../components/DiffPanel'; +import { renderDiffViewer, currentDiffLayout, rememberDiffLayout, type DiffViewerHandle } from '../components/DiffViewer'; import { renderChangeTree, renderChangeList, type ChangeTreeCallbacks } from './ChangeTree'; import { renderChangeItem } from './ChangeItem'; import { DiffStatProvider, type DiffStatLoadResult } from './DiffStatProvider'; @@ -569,12 +568,11 @@ export class SourceControlView { const toggleSlot = bar.createDiv({ cls: 'scv-detail-bar-toggle' }); // Shared DiffViewer renders an empty placeholder body; the async - // load below fills it (stale-guarded) once the diff content is ready. - // The viewer appends the body directly to `detail`, so the legacy - // .scv-detail-diff wrapper's CSS is kept by styling the body itself. - renderDiffViewer(detail, { - remote: '', - local: '', + // load below fills it in (stale-guarded) via the returned handle, + // once the diff content is ready. The viewer appends the body + // directly to `detail`, so the legacy .scv-detail-diff wrapper's CSS + // is kept by styling the body itself. + const viewer = renderDiffViewer(detail, { layout: currentDiffLayout(), toggleHost: toggleSlot, onLayoutChange: (next) => { @@ -584,12 +582,12 @@ export class SourceControlView { }); const diffBody = detail.querySelector('.scv-diff-tab-body'); diffBody?.addClass('scv-detail-diff'); - if (diffBody && this.selectedChangeId) { - void this.loadAndRenderDiff(diffBody, this.selectedChangeId); + if (this.selectedChangeId) { + void this.loadAndRenderDiff(viewer, this.selectedChangeId); } } - private async loadAndRenderDiff(container: HTMLElement, changeId: ChangeId): Promise { + private async loadAndRenderDiff(viewer: DiffViewerHandle, changeId: ChangeId): Promise { if (!this.callbacks.loadDiffContent) return; const item = this.viewModel.getState('all').items.find(i => i.id === changeId) ?? this.viewModel.getState('synced', true).items.find(i => i.id === changeId); @@ -598,7 +596,7 @@ export class SourceControlView { const content = await this.callbacks.loadDiffContent(item); // Stale response guard: the selection may have moved on while awaiting. if (!content || this.selectedChangeId !== changeId) return; - renderDiffPanel(container, content.remote, content.local); + viewer.setContent(content.remote, content.local); } private toggleFolder(path: string): void { diff --git a/styles.css b/styles.css index 2d782ee..05ca581 100644 --- a/styles.css +++ b/styles.css @@ -749,6 +749,8 @@ body.is-mobile .scv-view-toggle-label { display: none; } display: flex; flex-direction: column; min-height: 0; + min-width: 0; + max-width: 100%; } /* Let the diff fill all the space this full tab gives it, instead of the @@ -792,6 +794,8 @@ body.is-mobile .scv-view-toggle-label { display: none; } display: flex; flex-direction: column; height: 100%; + min-width: 0; + max-width: 100%; } .scv-detail-bar { @@ -891,11 +895,13 @@ body.is-mobile .scv-view-toggle-label { display: none; } .ssv-diff-grid { display: grid; - grid-template-columns: 1fr 1fr; + grid-template-columns: minmax(0, 1fr) minmax(0, 1fr); font-family: var(--font-monospace); font-size: 0.74em; max-height: 260px; - overflow-y: auto; + overflow: auto; + min-width: 0; + max-width: 100%; } .ssv-diff-hd { diff --git a/tests/setup.ts b/tests/setup.ts index 6cb2ec0..dd850fe 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -297,7 +297,7 @@ export const debounce = (cb: (...args: T) => unknown, timeo return fn; }; // Mutable so tests can exercise both the desktop and mobile branches. -export const Platform = { isDesktopApp: true, isMobile: false }; +export const Platform = { isDesktopApp: true, isMobile: false, isPhone: false, isTablet: false }; export const FileSystemAdapter = class { getBasePath() { return '/mock/path'; } }; diff --git a/tests/ui/components/DiffViewer.test.ts b/tests/ui/components/DiffViewer.test.ts index c63b50b..cc49092 100644 --- a/tests/ui/components/DiffViewer.test.ts +++ b/tests/ui/components/DiffViewer.test.ts @@ -1,4 +1,5 @@ -import { beforeAll, describe, expect, it } from 'vitest'; +import { afterEach, beforeAll, describe, expect, it } from 'vitest'; +import { Platform } from 'obsidian'; import { renderDiffViewer } from '../../../src/ui/components/DiffViewer'; import { setupObsidianDOM, createContainer } from '../setup-dom'; @@ -52,4 +53,53 @@ describe('renderDiffViewer', () => { expect(layouts).toEqual(['split', 'unified']); expect(body?.classList.contains('scv-diff-layout-unified')).toBe(true); }); + + describe('content-less initial render (async load in flight)', () => { + it('renders no diff panel when remote/local are omitted', () => { + const container = createContainer(); + + renderDiffViewer(container, { layout: 'split' }); + + expect(container.querySelector('.ssv-diff-split')).toBeNull(); + expect(container.querySelector('.ssv-diff-unified')).toBeNull(); + }); + + it('fills in the diff panel exactly once via the handle, replacing any prior content', () => { + const container = createContainer(); + + const viewer = renderDiffViewer(container, { layout: 'split' }); + viewer.setContent('remote text', 'local text'); + viewer.setContent('remote text 2', 'local text 2'); + + expect(container.querySelectorAll('.ssv-diff-split')).toHaveLength(1); + expect(container.querySelectorAll('.ssv-diff-unified')).toHaveLength(1); + expect(container.textContent).toContain('remote text 2'); + expect(container.textContent).not.toContain('remote text\n'); + }); + }); + + describe('phone layout policy', () => { + afterEach(() => { Platform.isPhone = false; }); + + it('forces unified and ignores the requested split layout', () => { + Platform.isPhone = true; + const container = createContainer(); + + renderDiffViewer(container, { remote: 'r', local: 'l', layout: 'split' }); + + const body = container.querySelector('.scv-diff-tab-body'); + expect(body?.classList.contains('scv-diff-layout-unified')).toBe(true); + expect(body?.classList.contains('scv-diff-layout-split')).toBe(false); + }); + + it('renders no layout toggle even when a toggleHost is given', () => { + Platform.isPhone = true; + const container = createContainer(); + const toggleHost = createContainer(); + + renderDiffViewer(container, { remote: 'r', local: 'l', layout: 'split', toggleHost }); + + expect(toggleHost.querySelector('.scv-diff-layout-toggle')).toBeNull(); + }); + }); }); \ No newline at end of file diff --git a/tests/ui/source-control/SourceControlView.test.ts b/tests/ui/source-control/SourceControlView.test.ts index 221e1c0..b650df2 100644 --- a/tests/ui/source-control/SourceControlView.test.ts +++ b/tests/ui/source-control/SourceControlView.test.ts @@ -645,7 +645,7 @@ describe('SourceControlView', () => { // onOpenDiff, and the host (SourceControlItemView) opens a main-area // tab. Only the mobile full-screen detail view still loads/renders // diff content inside SourceControlView itself. - afterEach(() => { Platform.isMobile = false; }); + afterEach(() => { Platform.isMobile = false; Platform.isPhone = false; }); it('loads and renders diff content in the mobile detail view for the clicked change', async () => { Platform.isMobile = true; @@ -666,6 +666,83 @@ describe('SourceControlView', () => { expect(container.querySelector('.ssv-diff-split')).not.toBeNull(); }); + it('renders no diff panel before the async load resolves, and exactly one once it does', async () => { + Platform.isMobile = true; + let resolveLoad: (value: { remote: string; local: string }) => void = () => {}; + const loadDiffContent = vi.fn().mockReturnValue(new Promise(resolve => { resolveLoad = resolve; })); + const { view } = buildView( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }], + { loadDiffContent }, + ); + view.render(container); + + (container.querySelector('.scv-change-item') as HTMLElement).click(); + await Promise.resolve(); + + expect(container.querySelector('.ssv-diff-split')).toBeNull(); + expect(container.querySelector('.ssv-diff-unified')).toBeNull(); + + resolveLoad({ remote: 'remote text', local: 'local text' }); + await Promise.resolve(); + await Promise.resolve(); + + expect(container.querySelectorAll('.ssv-diff-split')).toHaveLength(1); + expect(container.querySelectorAll('.ssv-diff-unified')).toHaveLength(1); + expect(container.querySelectorAll('.ssv-diff-hd')).toHaveLength(2); + }); + + it('does not let a stale async result render into a reopened detail view', async () => { + Platform.isMobile = true; + let resolveFirst: (value: { remote: string; local: string }) => void = () => {}; + const loadDiffContent = vi.fn() + .mockReturnValueOnce(new Promise(resolve => { resolveFirst = resolve; })) + .mockResolvedValueOnce({ remote: 'second remote', local: 'second local' }); + const { view } = buildView( + [ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }, + { id: toChangeId('c-2'), path: 'b.md', kind: 'local-modified' }, + ], + { loadDiffContent }, + ); + view.render(container); + + (container.querySelectorAll('.scv-change-item')[0] as HTMLElement).click(); + await Promise.resolve(); + (container.querySelector('.scv-detail-back') as HTMLElement).click(); + (container.querySelectorAll('.scv-change-item')[1] as HTMLElement).click(); + await Promise.resolve(); + await Promise.resolve(); + + resolveFirst({ remote: 'first remote', local: 'first local' }); + await Promise.resolve(); + await Promise.resolve(); + + expect(container.textContent).not.toContain('first remote'); + expect(container.textContent).toContain('second remote'); + expect(container.querySelectorAll('.ssv-diff-split')).toHaveLength(1); + }); + + it('forces unified with no toggle on a phone regardless of session split preference', async () => { + Platform.isMobile = true; + Platform.isPhone = true; + const loadDiffContent = vi.fn().mockResolvedValue({ remote: 'remote text', local: 'local text' }); + const { view } = buildView( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }], + { loadDiffContent }, + ); + view.render(container); + + (container.querySelector('.scv-change-item') as HTMLElement).click(); + await Promise.resolve(); + await Promise.resolve(); + + const diffContainer = container.querySelector('.scv-detail-diff'); + expect(diffContainer?.classList.contains('scv-diff-layout-unified')).toBe(true); + expect(container.querySelector('.scv-diff-layout-toggle')).toBeNull(); + + Platform.isPhone = false; + }); + it('does not render an inline diff pane on desktop -- only notifies onOpenDiff', () => { const { view } = buildView([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }]); view.render(container); From cf2239d84527e128aa38ec6be21f146bbac468e4 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 01:01:34 +0000 Subject: [PATCH 02/26] fix(docs): align agent guidance with source control architecture CLAUDE.md still described src/ui/SyncStatusView.ts as the plugin's main UI and never mentioned the Source Control surface that replaced it, so an agent reading it cold would look for a file that no longer exists and miss the real call chain (SourceControlItemView -> SourceControlView -> SourceControlActionService -> SyncWorkspace -> SyncManager/executors). Also documents the two compatibility identifiers (SOURCE_CONTROL_VIEW_TYPE = 'sync-status-view', the open-sync-status command id) as intentional, not leftover legacy code to clean up. --- CLAUDE.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 91607d5..d20bf5d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,7 +24,9 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co - **Settings**: `src/settings.ts` defines `GitLabFilesPushSettings` interface, `DEFAULT_SETTINGS` object, and `GitLabSyncSettingTab` for the Obsidian UI. - **Services**: `src/services/` abstracts the git provider behind `GitServiceInterface`, with `GitHubService` and `GitLabService` implementations sharing common logic via `BaseGitService`. - **Sync logic**: `src/logic/sync-manager.ts` handles push/pull, conflict detection, and rename detection; `src/logic/gitignore-manager.ts` merges local and remote `.gitignore` rules. -- **UI**: `src/ui/SyncStatusView.ts` renders the sync status side panel; `src/ui/components/` holds its sub-views. +- **UI**: the production Source Control surface is `SourceControlItemView` (`src/ui/source-control/SourceControlItemView.ts`), which renders `SourceControlView` (`src/ui/source-control/SourceControlView.ts`). User intent (push/pull/delete-remote/resolve-conflict) flows through `SourceControlActionService` (`src/logic/source-control/SourceControlActionService.ts`) into `SyncWorkspace` (`src/logic/sync/SyncWorkspace.ts`), which drives `SyncManager` and its executors (`PushExecutor`, `PullExecutor`, `RemoteDeleteExecutor`, etc. in `src/logic/sync/`). `src/ui/components/` holds shared diff/change presentation pieces used by this surface. + - Do not reintroduce `SyncStatusView` or `ui/sync-status/*` — that legacy presentation layer was replaced by the Source Control surface above and is blocked by an ESLint `no-restricted-imports` rule (`eslint.config.*`). The historical migration docs live in `docs/source-control-refactor/` and are marked as such; they are not current implementation guidance. + - `SOURCE_CONTROL_VIEW_TYPE` (`'sync-status-view'`) and the `open-sync-status` command id are intentionally kept as-is for pinned-leaf/workspace-layout compatibility — they resolve to the current `SourceControlItemView`, not a leftover of the old UI. Do not rename them as "cleanup." - **Bundling**: Uses `esbuild.config.mjs` for compilation from TypeScript to a single `main.js` file. - **Deployment**: Relies on `manifest.json` for plugin metadata and `versions.json` for version mapping/compatibility. From 8fcdfeaa95a945c76b9a379f360db9bd489fb4c0 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 01:01:54 +0000 Subject: [PATCH 03/26] fix(e2e): exercise current remote delete application path The remote-delete E2E called service.deleteFile() directly, with a comment saying it reproduced src/ui/SyncStatusView.ts's real call path -- but that view was removed. The production path is now SourceControlActionService.deleteRemote() -> SyncWorkspace.deleteRemote() -> RemoteDeleteExecutor -> gitService.deleteFile(), which also clears tracked metadata and the live status row as part of the same call, not as a separate manual step the way this test's old manager.clearMetadata() call implied. Rebuilds the test on a real SyncManagerWorkspace + SourceControlActionService, verified against a live Gitea sandbox (npm run test:e2e -- --provider gitea: 36 passed, 18 skipped). --- .../provider/suites/sync-manager.e2e.test.ts | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/e2e-tests/provider/suites/sync-manager.e2e.test.ts b/e2e-tests/provider/suites/sync-manager.e2e.test.ts index 57f16cc..718c068 100644 --- a/e2e-tests/provider/suites/sync-manager.e2e.test.ts +++ b/e2e-tests/provider/suites/sync-manager.e2e.test.ts @@ -4,12 +4,20 @@ import { SyncPlanModal, SyncPlanDirection } from '../../../src/ui/SyncPlanModal' import { BatchConflictResolutionModal } from '../../../src/ui/BatchConflictResolutionModal'; import { ObsidianSyncInteraction } from '../../../src/ui/ObsidianSyncInteraction'; import { describePushResult } from '../support/push-result-diagnostic'; +import { SyncManagerWorkspace } from '../../../src/logic/sync/SyncWorkspace'; +import { SourceControlActionService } from '../../../src/logic/source-control/SourceControlActionService'; +import { ChangeRepository } from '../../../src/logic/source-control/ChangeRepository'; +import { OperationState } from '../../../src/logic/source-control/OperationState'; +import { toChangeId } from '../../../src/logic/source-control/types'; // `import type` deliberately, not a value import: src/settings.ts also // exports settings-tab UI (GitLabSyncSettingTab -> FolderSuggest -> // AbstractInputSuggest etc.) which pulls in far more of `obsidian` than this // suite's minimal runtime shim provides. A type-only import is erased // entirely, so none of that module ever loads. import type { GitLabFilesPushSettings } from '../../../src/settings'; +import type { SyncStatusRefreshService } from '../../../src/logic/sync/SyncStatusRefreshService'; +import type { SyncDiffService } from '../../../src/logic/sync/SyncDiffService'; +import type { App } from 'obsidian'; import { TFile as ObsidianTFile } from 'obsidian'; import { GitVerifier } from '../support/git-verifier'; import { FakeVault, fakeApp, type TFileLike, type TFileCtor } from '../shim/fake-vault'; @@ -222,9 +230,14 @@ describe('SyncManager E2E', () => { expect(headAfterParent).toBe(headBefore); }); - it('deletes a file via the real service, verified independently', async () => { - // Deletion isn't a SyncManager method -- src/ui/SyncStatusView.ts calls - // gitService.deleteFile directly, so this reproduces that real path. + it('deletes a file via the current Source Control application path, verified independently', async () => { + // Deletion isn't a SyncManager method -- the production call chain is + // SourceControlActionService.deleteRemote() -> SyncWorkspace.deleteRemote() + // -> RemoteDeleteExecutor -> gitService.deleteFile(), not a direct + // provider call, so this exercises that full chain instead of + // bypassing it. `refreshService`/`diffService`/`app` are stubbed -- + // deleteRemote() never touches them -- the same pattern + // tests/logic/sync/SyncWorkspace.test.ts uses for its deleteRemote suite. const filePath = path('to-delete.md'); const vault = new FakeVault(TFile); vault.writeLocal(filePath, 'delete me'); @@ -235,9 +248,24 @@ describe('SyncManager E2E', () => { expect(initialPush.failed, describePushResult(initialPush)).toBe(0); expect(await verifier.fileMissing(filePath, branch)).toBe(false); - await service.deleteFile(filePath, branch, 'e2e: delete file'); - await manager.clearMetadata(filePath); + const changeId = toChangeId(filePath); + const repository = new ChangeRepository(); + repository.replace([{ id: changeId, path: filePath, kind: 'remote-only' }]); + const operations = new OperationState(); + const workspace = new SyncManagerWorkspace({ + manager: () => manager, + gitService: () => service, + settings: () => settings, + refreshService: {} as SyncStatusRefreshService, + diffService: {} as SyncDiffService, + normalizePath: p => p, + app: {} as App, + }); + const actionService = new SourceControlActionService(repository, operations, workspace); + + await actionService.deleteRemote([changeId]); + expect(operations.get(changeId)).toBe('success'); expect(await verifier.fileMissing(filePath, branch)).toBe(true); expect(settings.syncMetadata[filePath]).toBeUndefined(); }); From 0f11e172beec7b6d0bbf7a46141f5c5ea2c5429b Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 01:02:14 +0000 Subject: [PATCH 04/26] fix(docs): mark legacy source control migration docs historical docs/source-control-refactor/{roadmap,phase-1..4}.md describe an in-progress migration (roadmap.md dated 2026-08-22, still narrating uncommitted WIP) that has since landed on main in full -- nothing in that directory reflects the current implementation, but nothing marked it as historical either. Adds a banner to each pointing at the new docs/source-control.md, which describes only the current architecture and call chain without duplicating the old roadmap's narrative. --- .../phase-1-viewmodel-foundation.md | 3 ++ .../phase-2-action-unification.md | 3 ++ .../phase-3-source-control-ui.md | 3 ++ .../phase-4-legacy-cleanup.md | 3 ++ docs/source-control-refactor/roadmap.md | 8 +++- docs/source-control.md | 44 +++++++++++++++++++ 6 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 docs/source-control.md diff --git a/docs/source-control-refactor/phase-1-viewmodel-foundation.md b/docs/source-control-refactor/phase-1-viewmodel-foundation.md index ca14c94..cc23b50 100644 --- a/docs/source-control-refactor/phase-1-viewmodel-foundation.md +++ b/docs/source-control-refactor/phase-1-viewmodel-foundation.md @@ -1,5 +1,8 @@ # Phase 1 — Source Control ViewModel Foundation +> **Historical migration roadmap. Do not use as current implementation +> guidance.** See `docs/source-control.md` for the current architecture. + ## Goal 建立 Source Control UI 與 Sync domain 之間的 ViewModel layer。 diff --git a/docs/source-control-refactor/phase-2-action-unification.md b/docs/source-control-refactor/phase-2-action-unification.md index f9623bd..11b09ad 100644 --- a/docs/source-control-refactor/phase-2-action-unification.md +++ b/docs/source-control-refactor/phase-2-action-unification.md @@ -1,5 +1,8 @@ # Phase 2 — Sync Action Unification +> **Historical migration roadmap. Do not use as current implementation +> guidance.** See `docs/source-control.md` for the current architecture. + ## Goal 統一 Source Control、Context Menu、Single File 操作的 pipeline。 diff --git a/docs/source-control-refactor/phase-3-source-control-ui.md b/docs/source-control-refactor/phase-3-source-control-ui.md index f929917..1f89e9e 100644 --- a/docs/source-control-refactor/phase-3-source-control-ui.md +++ b/docs/source-control-refactor/phase-3-source-control-ui.md @@ -1,5 +1,8 @@ # Phase 3 — Source Control UI +> **Historical migration roadmap. Do not use as current implementation +> guidance.** See `docs/source-control.md` for the current architecture. + ## Goal 建立 VS Code style Source Control workflow。 diff --git a/docs/source-control-refactor/phase-4-legacy-cleanup.md b/docs/source-control-refactor/phase-4-legacy-cleanup.md index a1da710..56b4db1 100644 --- a/docs/source-control-refactor/phase-4-legacy-cleanup.md +++ b/docs/source-control-refactor/phase-4-legacy-cleanup.md @@ -1,5 +1,8 @@ # Phase 4 — Legacy Cleanup +> **Historical migration roadmap. Do not use as current implementation +> guidance.** See `docs/source-control.md` for the current architecture. + ## Goal 移除舊 Source Control orchestration，保留同步核心能力。 diff --git a/docs/source-control-refactor/roadmap.md b/docs/source-control-refactor/roadmap.md index a928ff3..d5cc3fd 100644 --- a/docs/source-control-refactor/roadmap.md +++ b/docs/source-control-refactor/roadmap.md @@ -1,8 +1,12 @@ # Source Control Refactor — Roadmap (v2) +> **Historical migration roadmap. Do not use as current implementation +> guidance.** The migration this document tracked has landed on `main`; for +> the current architecture see `docs/source-control.md`. + > Supersedes `phase-1..4-*.md`. Those phase docs are kept only as historical -> design notes; this file is the authoritative current plan, grounded in the -> actual branch state as of 2026-08-22. +> design notes; this file was the authoritative current plan as of +> 2026-08-22, before the migration it tracked landed on `main`. ## Where we actually are diff --git a/docs/source-control.md b/docs/source-control.md new file mode 100644 index 0000000..93ed5fb --- /dev/null +++ b/docs/source-control.md @@ -0,0 +1,44 @@ +# Source Control — Current Architecture + +The Source Control side panel is the plugin's only sync UI. There is no +separate "sync status" view; `docs/source-control-refactor/` describes the +historical migration into this architecture and is not current guidance. + +## Call chain + +``` +SourceControlItemView (src/ui/source-control/SourceControlItemView.ts) + └─ SourceControlView (src/ui/source-control/SourceControlView.ts) + └─ SourceControlActionService (src/logic/source-control/SourceControlActionService.ts) + └─ SyncWorkspace (src/logic/sync/SyncWorkspace.ts) + └─ SyncManager + executors (src/logic/sync/, e.g. PushExecutor, + PullExecutor, RemoteDeleteExecutor) +``` + +- `SourceControlItemView` is the `ItemView` Obsidian mounts; it owns no + rendering logic itself and delegates to `SourceControlView`. +- `SourceControlView` renders the change tree, Sync Queue, and diff surfaces + (`src/ui/components/`, `src/ui/source-control/DiffTabView.ts`), and turns + clicks into calls on `SourceControlActionService`. +- `SourceControlActionService` converts Source Control intent (push / pull / + delete-remote / delete-local / resolve-conflict) into `SyncWorkspace` calls + and reports outcome via `OperationState`. It never talks to a git provider + directly. +- `SyncWorkspace` is the execution boundary: it drives the real `SyncManager` + and provider-mutating executors (`PushExecutor`, `PullExecutor`, + `RemoteDeleteExecutor`, etc.), which in turn call `GitServiceInterface` + (`src/services/`). + +## Compatibility identifiers (do not remove) + +- `SOURCE_CONTROL_VIEW_TYPE = 'sync-status-view'` — kept so pinned leaves and + saved workspace layouts from before the Source Control migration resolve to + the current `SourceControlItemView` instead of breaking. +- The `open-sync-status` command id — same reason; it already routes to + `activateSourceControlView()`. + +## Legacy surface (removed, do not reintroduce) + +`SyncStatusView` and `ui/sync-status/*` were the pre-migration UI and no +longer exist in `src/`. An ESLint `no-restricted-imports` rule +(`eslint.config.*`) blocks reintroducing imports from those paths. From beba48dd1997221d2291227080fedf80f9e42101 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 01:02:34 +0000 Subject: [PATCH 05/26] fix(test): guard removed sync status presentation imports Two regression guards so a future refactor can't silently undo this cleanup: eslint.config.mts's no-restricted-imports rule blocking ui/sync-status and SyncStatusView imports is now asserted directly (it existed before this PR but had no test locking it in), and the remote delete E2E is now locked to keep going through SourceControlActionService/SyncWorkspace rather than quietly reverting to a direct service.deleteFile() provider bypass. --- tests/ci-workflow.test.ts | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/ci-workflow.test.ts b/tests/ci-workflow.test.ts index ce85aea..4d3975f 100644 --- a/tests/ci-workflow.test.ts +++ b/tests/ci-workflow.test.ts @@ -7,6 +7,7 @@ const harness = readFileSync('scripts/e2e-harness.sh', 'utf8'); const runner = readFileSync('scripts/run-e2e.sh', 'utf8'); const vitestE2eConfig = readFileSync('vitest.e2e.config.ts', 'utf8'); const eslintConfig = readFileSync('eslint.config.mts', 'utf8'); +const syncManagerE2eSuite = readFileSync('e2e-tests/provider/suites/sync-manager.e2e.test.ts', 'utf8'); describe('CI workflow contracts', () => { it('retries transient provider failures three times', () => { @@ -131,4 +132,29 @@ describe('E2E scanner-boundary contracts (e2e-tests/provider, no runtime generat expect(eslintConfig).toContain('"e2e-tests/**/*.ts"'); expect(eslintConfig).not.toContain('"e2e/**/*.ts"'); }); + + it('still blocks src/ imports of the removed legacy sync-status presentation layer', () => { + // Architecture regression guard for the SyncStatusView -> Source + // Control migration (see docs/source-control.md): a future refactor + // must not silently drop this no-restricted-imports rule and let + // ui/sync-status or SyncStatusView get re-wired back in. + expect(eslintConfig).toContain('"**/ui/sync-status"'); + expect(eslintConfig).toContain('"**/ui/sync-status/*"'); + expect(eslintConfig).toContain('"**/SyncStatusView"'); + expect(eslintConfig).toContain('"**/ui/SyncStatusView"'); + expect(eslintConfig).toContain('no-restricted-imports'); + }); + + it('exercises remote delete through the Source Control application path, not a direct provider bypass', () => { + // The remote-delete E2E used to call `service.deleteFile()` directly, + // reproducing what the removed SyncStatusView UI used to do. The + // current production path is SourceControlActionService.deleteRemote() + // -> SyncWorkspace.deleteRemote() -> RemoteDeleteExecutor -> + // gitService.deleteFile() -- a future edit must keep exercising that + // chain instead of quietly reverting to the raw provider call. + expect(syncManagerE2eSuite).not.toMatch(/\bservice\.deleteFile\(/); + expect(syncManagerE2eSuite).toContain('actionService.deleteRemote('); + expect(syncManagerE2eSuite).toContain("from '../../../src/logic/source-control/SourceControlActionService'"); + expect(syncManagerE2eSuite).toContain("from '../../../src/logic/sync/SyncWorkspace'"); + }); }); From 6baa0da4672fdfddc557d54edd4eb44e59d1f4e2 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 02:16:25 +0000 Subject: [PATCH 06/26] fix(source-control): preserve per-change action overrides in sync selection Adds resolveSyncAction/availableSyncActions to ChangeActionPolicy and per-change action override tracking to SyncSelectionStore, so an explicit user choice (e.g. pull instead of the default push) can survive selection state without the store having to know change-kind legality itself. Not yet wired into the UI or ActionService. --- .../source-control/ChangeActionPolicy.ts | 43 +++++++++++- .../source-control/SyncSelectionStore.ts | 29 +++++++- .../source-control/ChangeActionPolicy.test.ts | 61 +++++++++++++++- .../source-control/SyncSelectionStore.test.ts | 70 +++++++++++++++++++ 4 files changed, 198 insertions(+), 5 deletions(-) diff --git a/src/logic/source-control/ChangeActionPolicy.ts b/src/logic/source-control/ChangeActionPolicy.ts index c36bbb7..e983528 100644 --- a/src/logic/source-control/ChangeActionPolicy.ts +++ b/src/logic/source-control/ChangeActionPolicy.ts @@ -9,9 +9,9 @@ import type { SyncChangeKind } from './types'; * which primitive a change kind maps to isn't a rendering concern — so it * lives here instead, decoupled from presentation. */ -export type DefaultSyncAction = 'push' | 'pull' | 'delete-remote'; +export type SyncAction = 'push' | 'pull' | 'delete-remote'; -const DEFAULT_ACTION: Record = { +const DEFAULT_ACTION: Record = { 'local-only': 'push', 'local-modified': 'push', // A tracked file removed locally has no local content to push, so its @@ -29,8 +29,45 @@ const DEFAULT_ACTION: Record = { synced: 'push', }; +// Every action a change kind may legally resolve to, default first. Drives +// both what an explicit override is allowed to be and the fallback when a +// stored override no longer applies (see `resolveSyncAction`). `conflict` +// and `synced` intentionally allow only their default: conflict resolution +// runs through `BatchConflictResolutionModal`, not a Queue override, and +// `synced` never reaches the Sync Queue at all. +const AVAILABLE_ACTIONS: Record = { + 'local-only': ['push'], + 'local-modified': ['push', 'pull'], + 'local-deleted': ['delete-remote', 'pull'], + 'remote-only': ['pull', 'delete-remote'], + 'remote-modified': ['pull', 'push'], + moved: ['push'], + conflict: ['push'], + synced: ['push'], +}; + /** The default sync action a change kind routes to when synced from the Sync Queue. */ -export function defaultSyncAction(kind: SyncChangeKind): DefaultSyncAction { +export function defaultSyncAction(kind: SyncChangeKind): SyncAction { + return DEFAULT_ACTION[kind]; +} + +/** Every action a change kind may legally resolve to, default first. */ +export function availableSyncActions(kind: SyncChangeKind): readonly SyncAction[] { + return AVAILABLE_ACTIONS[kind]; +} + +/** + * Resolves the action a change actually syncs as: the given override if it's + * still legal for `kind`, otherwise the kind's default. This is what makes a + * stale override (e.g. the user picked "pull" on a `local-modified` change, + * then it became `local-only` after a remote delete) harmless — it silently + * falls back instead of ever executing an action the current kind can't + * support. + */ +export function resolveSyncAction(kind: SyncChangeKind, override?: SyncAction): SyncAction { + if (override && AVAILABLE_ACTIONS[kind].includes(override)) { + return override; + } return DEFAULT_ACTION[kind]; } diff --git a/src/logic/source-control/SyncSelectionStore.ts b/src/logic/source-control/SyncSelectionStore.ts index b1a7060..9a7c3bd 100644 --- a/src/logic/source-control/SyncSelectionStore.ts +++ b/src/logic/source-control/SyncSelectionStore.ts @@ -1,4 +1,5 @@ import type { ChangeId } from './types'; +import type { SyncAction } from './ChangeActionPolicy'; /** * Tracks which pending sync changes are selected for the Sync Queue — @@ -11,9 +12,16 @@ import type { ChangeId } from './types'; * * Keyed by ChangeId rather than path so a rename/move doesn't drop the * selection. + * + * Also holds an optional per-change action override — the user explicitly + * picking pull instead of the default push, say — keyed the same way. + * Legality of an override (is 'pull' even valid for this change's kind) is + * not this store's concern; that's `ChangeActionPolicy`'s job, applied at + * read time via `resolveSyncAction`. */ export class SyncSelectionStore { private readonly selected = new Set(); + private readonly actionOverrides = new Map(); selectForSync(changeId: ChangeId): void { this.selected.add(changeId); @@ -21,6 +29,7 @@ export class SyncSelectionStore { deselectFromSync(changeId: ChangeId): void { this.selected.delete(changeId); + this.actionOverrides.delete(changeId); } /** Selects a batch of changes for sync in one call (folder "select all"). */ @@ -30,7 +39,10 @@ export class SyncSelectionStore { /** Deselects a batch of changes from sync in one call ("clear queue" / folder deselect). */ deselectMany(changeIds: readonly ChangeId[]): void { - for (const id of changeIds) this.selected.delete(id); + for (const id of changeIds) { + this.selected.delete(id); + this.actionOverrides.delete(id); + } } isIncluded(changeId: ChangeId): boolean { @@ -41,12 +53,27 @@ export class SyncSelectionStore { return [...this.selected]; } + /** Records the user's explicit action choice for a change (e.g. pull instead of the default push). */ + setActionOverride(changeId: ChangeId, action: SyncAction): void { + this.actionOverrides.set(changeId, action); + } + + /** Reverts a change back to its default action. */ + clearActionOverride(changeId: ChangeId): void { + this.actionOverrides.delete(changeId); + } + + getActionOverride(changeId: ChangeId): SyncAction | undefined { + return this.actionOverrides.get(changeId); + } + /** Drops selections for change ids that are no longer present, keeping the rest. */ refresh(currentChangeIds: readonly ChangeId[]): void { const present = new Set(currentChangeIds); for (const changeId of this.selected) { if (!present.has(changeId)) { this.selected.delete(changeId); + this.actionOverrides.delete(changeId); } } } diff --git a/tests/logic/source-control/ChangeActionPolicy.test.ts b/tests/logic/source-control/ChangeActionPolicy.test.ts index f958a84..31cdb97 100644 --- a/tests/logic/source-control/ChangeActionPolicy.test.ts +++ b/tests/logic/source-control/ChangeActionPolicy.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from 'vitest'; -import { canDownload, defaultSyncAction } from '../../../src/logic/source-control/ChangeActionPolicy'; +import { + availableSyncActions, + canDownload, + defaultSyncAction, + resolveSyncAction, +} from '../../../src/logic/source-control/ChangeActionPolicy'; import type { SyncChangeKind } from '../../../src/logic/source-control/types'; describe('defaultSyncAction', () => { @@ -64,3 +69,57 @@ describe('canDownload', () => { expect(canDownload('synced')).toBe(false); }); }); + +describe('availableSyncActions', () => { + it('lists the default first', () => { + expect(availableSyncActions('local-modified')[0]).toBe(defaultSyncAction('local-modified')); + expect(availableSyncActions('remote-only')[0]).toBe(defaultSyncAction('remote-only')); + expect(availableSyncActions('local-deleted')[0]).toBe(defaultSyncAction('local-deleted')); + }); + + it('allows push and pull for local-modified (push local, or use remote instead)', () => { + expect(availableSyncActions('local-modified')).toEqual(['push', 'pull']); + }); + + it('allows pull and delete-remote for remote-only (download, or delete it remotely)', () => { + expect(availableSyncActions('remote-only')).toEqual(['pull', 'delete-remote']); + }); + + it('allows pull and push for remote-modified (use remote, or overwrite with local)', () => { + expect(availableSyncActions('remote-modified')).toEqual(['pull', 'push']); + }); + + it('allows delete-remote and pull for local-deleted (mirror the delete, or restore it)', () => { + expect(availableSyncActions('local-deleted')).toEqual(['delete-remote', 'pull']); + }); + + it('only allows the default for local-only, moved, conflict, and synced', () => { + expect(availableSyncActions('local-only')).toEqual(['push']); + expect(availableSyncActions('moved')).toEqual(['push']); + expect(availableSyncActions('conflict')).toEqual(['push']); + expect(availableSyncActions('synced')).toEqual(['push']); + }); +}); + +describe('resolveSyncAction', () => { + it('returns the default when no override is given', () => { + expect(resolveSyncAction('local-modified')).toBe('push'); + expect(resolveSyncAction('remote-only')).toBe('pull'); + }); + + it('honors a legal override', () => { + expect(resolveSyncAction('local-modified', 'pull')).toBe('pull'); + expect(resolveSyncAction('remote-only', 'delete-remote')).toBe('delete-remote'); + }); + + it('falls back to the default when the override is no longer legal for the kind', () => { + // e.g. stored override was 'pull' while the change was local-modified, + // then it became local-only (remote copy deleted) — 'pull' can't apply anymore. + expect(resolveSyncAction('local-only', 'pull')).toBe('push'); + }); + + it('falls back to the default for kinds that only allow their default', () => { + expect(resolveSyncAction('conflict', 'pull')).toBe('push'); + expect(resolveSyncAction('moved', 'pull')).toBe('push'); + }); +}); diff --git a/tests/logic/source-control/SyncSelectionStore.test.ts b/tests/logic/source-control/SyncSelectionStore.test.ts index 9b671bf..39f2fce 100644 --- a/tests/logic/source-control/SyncSelectionStore.test.ts +++ b/tests/logic/source-control/SyncSelectionStore.test.ts @@ -96,4 +96,74 @@ describe('SyncSelectionStore', () => { expect(store.getSelectedChangeIds()).toEqual([toChangeId('change-a')]); }); }); + + describe('action overrides', () => { + it('has no override by default', () => { + const store = new SyncSelectionStore(); + store.selectForSync(toChangeId('change-a')); + + expect(store.getActionOverride(toChangeId('change-a'))).toBeUndefined(); + }); + + it('records and returns an explicit override', () => { + const store = new SyncSelectionStore(); + store.selectForSync(toChangeId('change-a')); + + store.setActionOverride(toChangeId('change-a'), 'pull'); + + expect(store.getActionOverride(toChangeId('change-a'))).toBe('pull'); + }); + + it('clearActionOverride reverts to no override', () => { + const store = new SyncSelectionStore(); + store.selectForSync(toChangeId('change-a')); + store.setActionOverride(toChangeId('change-a'), 'pull'); + + store.clearActionOverride(toChangeId('change-a')); + + expect(store.getActionOverride(toChangeId('change-a'))).toBeUndefined(); + }); + + it('deselectFromSync clears the override along with the selection', () => { + const store = new SyncSelectionStore(); + store.selectForSync(toChangeId('change-a')); + store.setActionOverride(toChangeId('change-a'), 'pull'); + + store.deselectFromSync(toChangeId('change-a')); + + expect(store.getActionOverride(toChangeId('change-a'))).toBeUndefined(); + }); + + it('deselectMany clears overrides for all deselected ids', () => { + const store = new SyncSelectionStore(); + store.selectMany([toChangeId('change-a'), toChangeId('change-b')]); + store.setActionOverride(toChangeId('change-a'), 'pull'); + store.setActionOverride(toChangeId('change-b'), 'delete-remote'); + + store.deselectMany([toChangeId('change-a'), toChangeId('change-b')]); + + expect(store.getActionOverride(toChangeId('change-a'))).toBeUndefined(); + expect(store.getActionOverride(toChangeId('change-b'))).toBeUndefined(); + }); + + it('refresh clears the override for a change id that is no longer present', () => { + const store = new SyncSelectionStore(); + store.selectForSync(toChangeId('change-a')); + store.setActionOverride(toChangeId('change-a'), 'pull'); + + store.refresh([]); + + expect(store.getActionOverride(toChangeId('change-a'))).toBeUndefined(); + }); + + it('refresh keeps the override for a change id that is still present', () => { + const store = new SyncSelectionStore(); + store.selectForSync(toChangeId('change-a')); + store.setActionOverride(toChangeId('change-a'), 'pull'); + + store.refresh([toChangeId('change-a')]); + + expect(store.getActionOverride(toChangeId('change-a'))).toBe('pull'); + }); + }); }); From 601013fe69b186f32aef4f39ae7f3a4aff56592e Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 02:22:49 +0000 Subject: [PATCH 07/26] fix(source-control): resolve queue grouping from actual sync action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SourceControlItem now carries a resolved syncAction (override if still legal for the current kind, else the kind default) and hasActionOverride, projected in SourceControlViewModel with stale-override cleanup baked in. Sync Queue grouping (Upload/Download/Delete) now reads item.syncAction instead of recomputing defaultSyncAction(item.kind), so a queue with overridden items groups by what Sync will actually do. Action execution (ActionService, SyncPlan) still ignores the override — that's the next commit. --- .../source-control/SourceControlViewModel.ts | 25 ++++++++++- .../source-control/SourceControlItemView.ts | 3 ++ src/ui/source-control/SourceControlView.ts | 20 ++++----- .../SourceControlActionService.test.ts | 4 ++ .../SourceControlViewModel.test.ts | 42 +++++++++++++++++++ .../source-control/ChangePresentation.test.ts | 9 +++- tests/ui/source-control/ChangeTree.test.ts | 9 +++- .../source-control/DiffStatProvider.test.ts | 11 ++++- .../source-control/SourceControlView.test.ts | 18 ++++++++ 9 files changed, 127 insertions(+), 14 deletions(-) diff --git a/src/logic/source-control/SourceControlViewModel.ts b/src/logic/source-control/SourceControlViewModel.ts index d457c9d..a3964bc 100644 --- a/src/logic/source-control/SourceControlViewModel.ts +++ b/src/logic/source-control/SourceControlViewModel.ts @@ -1,4 +1,5 @@ import type { ChangeRepository } from './ChangeRepository'; +import { resolveSyncAction, type SyncAction } from './ChangeActionPolicy'; import { buildSummary, type SourceControlCounts } from './SourceControlSummary'; import type { OperationState, OperationStatus } from './OperationState'; import type { RefreshReason } from './RefreshReason'; @@ -15,6 +16,10 @@ export interface SourceControlItem { kind: SyncChangeKind; isSelectedForSync: boolean; operationStatus: OperationStatus; + /** The action this change actually syncs as — the user's override if still legal for `kind`, otherwise the default. */ + syncAction: SyncAction; + /** Whether `syncAction` came from a still-legal user override, as opposed to the kind's default. */ + hasActionOverride: boolean; } /** The complete state the Source Control UI needs to render for a given filter. */ @@ -50,7 +55,13 @@ export interface SourceControlViewState { * synced count is reported as `0` and the `synced` filter yields no items, * matching the "Show synced" toggle (default off). * - * The one non-projection responsibility is {@link refresh}: it delegates to an + * `toItem` has one side effect for the same reason `refresh` does: a stale + * action override (recorded when a change was e.g. `local-modified`, now + * stranded because the change became `local-only`) is cleared on the + * selection store as soon as a projection notices it's no longer legal, + * rather than left to resurface if the kind later reverts. + * + * The other non-projection responsibility is {@link refresh}: it delegates to an * injected refresh callback (wired to `SyncWorkspace.refresh()` in `main.ts`) * and drives the injected {@link RefreshState} holder so the UI can surface * loading/failed states. It holds no provider or refresh logic of its own, @@ -119,6 +130,16 @@ export class SourceControlViewModel { } private toItem(change: SyncChange): SourceControlItem { + const storedOverride = this.selectionStore.getActionOverride(change.id); + const syncAction = resolveSyncAction(change.kind, storedOverride); + const hasActionOverride = storedOverride !== undefined && storedOverride === syncAction; + // The change's kind moved on since the override was recorded (e.g. a + // stored 'pull' on what's now local-only) — resolveSyncAction already + // fell back to the default, so drop the now-meaningless override + // rather than let it linger and resurface once the kind reverts. + if (storedOverride !== undefined && !hasActionOverride) { + this.selectionStore.clearActionOverride(change.id); + } return { id: change.id, path: change.path, @@ -126,6 +147,8 @@ export class SourceControlViewModel { kind: change.kind, isSelectedForSync: this.selectionStore.isIncluded(change.id), operationStatus: this.operations.get(change.id), + syncAction, + hasActionOverride, }; } } \ No newline at end of file diff --git a/src/ui/source-control/SourceControlItemView.ts b/src/ui/source-control/SourceControlItemView.ts index 6a583b4..0842bd8 100644 --- a/src/ui/source-control/SourceControlItemView.ts +++ b/src/ui/source-control/SourceControlItemView.ts @@ -2,6 +2,7 @@ import { ItemView, Platform, TFile, WorkspaceLeaf, debounce } from 'obsidian'; import GitLabFilesPush from '../../main'; import { t } from '../../i18n'; import type { SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; +import { resolveSyncAction } from '../../logic/source-control/ChangeActionPolicy'; import type { FileStatus } from '../../logic/sync-status-service'; import { toChangeId } from '../../logic/source-control/types'; import { SourceControlView, type SourceControlViewCallbacks } from './SourceControlView'; @@ -125,6 +126,8 @@ export class SourceControlItemView extends ItemView { ...change, isSelectedForSync: false, operationStatus: 'idle', + syncAction: resolveSyncAction(change.kind), + hasActionOverride: false, }; const content = await this.plugin.sourceControlActions.loadDiffContent(item); if (requestId !== this.diffTabRequestSeq) return; diff --git a/src/ui/source-control/SourceControlView.ts b/src/ui/source-control/SourceControlView.ts index 5a6de09..406cd95 100644 --- a/src/ui/source-control/SourceControlView.ts +++ b/src/ui/source-control/SourceControlView.ts @@ -3,7 +3,6 @@ import { t } from '../../i18n'; import type { SourceControlFilter } from '../../logic/source-control/SourceControlFilter'; import { SourceControlViewModel, type SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; import type { ChangeId } from '../../logic/source-control/types'; -import { defaultSyncAction } from '../../logic/source-control/ChangeActionPolicy'; import { ICONS } from '../components/icons'; import { renderDiffViewer, currentDiffLayout, rememberDiffLayout, type DiffViewerHandle } from '../components/DiffViewer'; import { renderChangeTree, renderChangeList, type ChangeTreeCallbacks } from './ChangeTree'; @@ -504,15 +503,16 @@ export class SourceControlView { text: t('sourceControl.section.queueSubtitle', { count: syncQueue.length }), }); const list = section.createDiv({ cls: 'scv-selected-section-list' }); - // Group the queue by its default sync action so a mixed batch reads - // as what the Sync button will actually do (Upload / Download / - // Delete) rather than a flat list of ambiguous badges. Only surface - // group labels when more than one action is present in the batch — - // a single-action queue stays flat (no label noise) and matches the - // pre-categorization layout. - const upload = syncQueue.filter(item => defaultSyncAction(item.kind) === 'push'); - const download = syncQueue.filter(item => defaultSyncAction(item.kind) === 'pull'); - const deleteRemote = syncQueue.filter(item => defaultSyncAction(item.kind) === 'delete-remote'); + // Group the queue by its resolved sync action (the default, unless + // the user overrode it) so a mixed batch reads as what the Sync + // button will actually do (Upload / Download / Delete) rather than a + // flat list of ambiguous badges. Only surface group labels when more + // than one action is present in the batch — a single-action queue + // stays flat (no label noise) and matches the pre-categorization + // layout. + const upload = syncQueue.filter(item => item.syncAction === 'push'); + const download = syncQueue.filter(item => item.syncAction === 'pull'); + const deleteRemote = syncQueue.filter(item => item.syncAction === 'delete-remote'); const groupCount = [upload, download, deleteRemote].filter(group => group.length > 0).length; const mixed = groupCount > 1; if (mixed && upload.length > 0) list.createDiv({ cls: 'scv-queue-group-label', text: t('sourceControl.queue.upload') }); diff --git a/tests/logic/source-control/SourceControlActionService.test.ts b/tests/logic/source-control/SourceControlActionService.test.ts index a273733..3ae6b23 100644 --- a/tests/logic/source-control/SourceControlActionService.test.ts +++ b/tests/logic/source-control/SourceControlActionService.test.ts @@ -658,6 +658,8 @@ describe('SourceControlActionService', () => { kind: 'local-modified', isSelectedForSync: false, operationStatus: 'idle', + syncAction: 'push', + hasActionOverride: false, }); expect(getDiff).toHaveBeenCalledWith('a.md'); @@ -682,6 +684,8 @@ describe('SourceControlActionService', () => { kind: 'local-modified', isSelectedForSync: false, operationStatus: 'idle', + syncAction: 'push', + hasActionOverride: false, }); expect(content).toBeNull(); diff --git a/tests/logic/source-control/SourceControlViewModel.test.ts b/tests/logic/source-control/SourceControlViewModel.test.ts index cb10358..ff9c963 100644 --- a/tests/logic/source-control/SourceControlViewModel.test.ts +++ b/tests/logic/source-control/SourceControlViewModel.test.ts @@ -198,4 +198,46 @@ describe('SourceControlViewModel', () => { await expect(viewModel.refresh()).rejects.toThrow('boom'); expect(refreshState.get()).toBe('failed'); }); + + describe('syncAction projection', () => { + it('defaults syncAction to the kind default with no override', () => { + const { viewModel } = buildViewModel([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }]); + + const item = viewModel.getState('all').items[0]; + expect(item?.syncAction).toBe('push'); + expect(item?.hasActionOverride).toBe(false); + }); + + it('resolves syncAction to a legal override and marks hasActionOverride', () => { + const { viewModel, selection } = buildViewModel([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }]); + selection.setActionOverride(toChangeId('c-1'), 'pull'); + + const item = viewModel.getState('all').items[0]; + expect(item?.syncAction).toBe('pull'); + expect(item?.hasActionOverride).toBe(true); + }); + + it('falls back to the default and clears a stale override once the kind no longer supports it', () => { + const repository = new ChangeRepository(); + repository.replace([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }]); + const selection = new SyncSelectionStore(); + selection.setActionOverride(toChangeId('c-1'), 'pull'); + const viewModel = new SourceControlViewModel( + repository, + selection, + new OperationState(), + vi.fn().mockResolvedValue(undefined), + new RefreshState(), + ); + + // Remote copy of the change disappears — kind moves from + // local-modified (allows pull) to local-only (push only). + repository.replace([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }]); + + const item = viewModel.getState('all').items[0]; + expect(item?.syncAction).toBe('push'); + expect(item?.hasActionOverride).toBe(false); + expect(selection.getActionOverride(toChangeId('c-1'))).toBeUndefined(); + }); + }); }); diff --git a/tests/ui/source-control/ChangePresentation.test.ts b/tests/ui/source-control/ChangePresentation.test.ts index d3d47f0..358f1ce 100644 --- a/tests/ui/source-control/ChangePresentation.test.ts +++ b/tests/ui/source-control/ChangePresentation.test.ts @@ -1,13 +1,20 @@ import { describe, expect, it, beforeAll } from 'vitest'; import { addedContentStat, cheapLocalStat, computeDiffStat, deletedContentStat, presentChange } from '../../../src/ui/source-control/ChangePresentation'; import type { SourceControlItem } from '../../../src/logic/source-control/SourceControlViewModel'; +import { resolveSyncAction } from '../../../src/logic/source-control/ChangeActionPolicy'; import { toChangeId } from '../../../src/logic/source-control/types'; import { setupObsidianDOM } from '../setup-dom'; beforeAll(() => { setupObsidianDOM(); }); function item(overrides: Partial & Pick): SourceControlItem { - return { isSelectedForSync: false, operationStatus: 'idle', ...overrides }; + return { + isSelectedForSync: false, + operationStatus: 'idle', + syncAction: resolveSyncAction(overrides.kind), + hasActionOverride: false, + ...overrides, + }; } describe('presentChange', () => { diff --git a/tests/ui/source-control/ChangeTree.test.ts b/tests/ui/source-control/ChangeTree.test.ts index d910f91..b0f9aa6 100644 --- a/tests/ui/source-control/ChangeTree.test.ts +++ b/tests/ui/source-control/ChangeTree.test.ts @@ -1,13 +1,20 @@ import { describe, expect, it, vi, beforeAll, beforeEach } from 'vitest'; import { renderChangeTree, type ChangeTreeCallbacks } from '../../../src/ui/source-control/ChangeTree'; import type { SourceControlItem } from '../../../src/logic/source-control/SourceControlViewModel'; +import { resolveSyncAction } from '../../../src/logic/source-control/ChangeActionPolicy'; import { toChangeId } from '../../../src/logic/source-control/types'; import { setupObsidianDOM, createContainer } from '../setup-dom'; beforeAll(() => { setupObsidianDOM(); }); function item(overrides: Partial & Pick): SourceControlItem { - return { isSelectedForSync: false, operationStatus: 'idle', ...overrides }; + return { + isSelectedForSync: false, + operationStatus: 'idle', + syncAction: resolveSyncAction(overrides.kind), + hasActionOverride: false, + ...overrides, + }; } describe('renderChangeTree', () => { diff --git a/tests/ui/source-control/DiffStatProvider.test.ts b/tests/ui/source-control/DiffStatProvider.test.ts index 0c07963..73e8938 100644 --- a/tests/ui/source-control/DiffStatProvider.test.ts +++ b/tests/ui/source-control/DiffStatProvider.test.ts @@ -2,11 +2,20 @@ import { describe, expect, it, vi } from 'vitest'; import { DiffStatProvider } from '../../../src/ui/source-control/DiffStatProvider'; import type { DiffStatLoadResult } from '../../../src/ui/source-control/DiffStatProvider'; import type { SourceControlItem } from '../../../src/logic/source-control/SourceControlViewModel'; +import { resolveSyncAction } from '../../../src/logic/source-control/ChangeActionPolicy'; import type { ChangeStat } from '../../../src/ui/source-control/ChangePresentation'; import { toChangeId } from '../../../src/logic/source-control/types'; function item(id: string, kind: SourceControlItem['kind'] = 'local-only'): SourceControlItem { - return { id: toChangeId(id), path: `${id}.md`, kind, isSelectedForSync: false, operationStatus: 'idle' }; + return { + id: toChangeId(id), + path: `${id}.md`, + kind, + isSelectedForSync: false, + operationStatus: 'idle', + syncAction: resolveSyncAction(kind), + hasActionOverride: false, + }; } function ready(stat: ChangeStat): DiffStatLoadResult { diff --git a/tests/ui/source-control/SourceControlView.test.ts b/tests/ui/source-control/SourceControlView.test.ts index b650df2..b1ad8ca 100644 --- a/tests/ui/source-control/SourceControlView.test.ts +++ b/tests/ui/source-control/SourceControlView.test.ts @@ -521,6 +521,24 @@ describe('SourceControlView', () => { expect(container.querySelector('.scv-queue-group-label')).toBeNull(); }); + + it('groups by the resolved action, not the kind default, when the user overrides it', () => { + const { view, selection } = buildView( + [ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }, + { id: toChangeId('c-2'), path: 'b.md', kind: 'local-modified' }, + ], + ); + selection.selectForSync(toChangeId('c-1')); + selection.selectForSync(toChangeId('c-2')); + // Both default to push; overriding one to pull should move it into + // Download despite sharing the same change kind as the other. + selection.setActionOverride(toChangeId('c-2'), 'pull'); + view.render(container); + + const labels = Array.from(container.querySelectorAll('.scv-queue-group-label')).map(el => el.textContent); + expect(labels).toEqual(['Upload', 'Download']); + }); }); describe('inline download action', () => { From 735bdc0627918734c9494093d822e14b4b9063b4 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 02:28:51 +0000 Subject: [PATCH 08/26] fix(source-control): honor explicit actions in sync queue execution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SourceControlActionService.sync() now takes SyncIntentRequest[] (changeId + optional action) instead of bare ChangeId[], resolving each via ChangeActionPolicy.resolveSyncAction against the change's current kind before bucketing into push/pull/delete-remote — so a stale intent degrades to the kind's default rather than forcing an illegal action. SourceControlView.runSync threads an item's override through only when hasActionOverride is set; plain queue items still sync via the default. Conflict resolution, plan merging, and the single-commit contract are unchanged. --- .../suites/source-control-flows.e2e.test.ts | 4 +- .../support/two-client-sync-scenario.ts | 4 +- .../SourceControlActionService.ts | 70 ++++++--- src/ui/source-control/SourceControlView.ts | 8 +- .../SourceControlActionService.test.ts | 133 ++++++++++++++++-- .../SourceControlItemView.test.ts | 4 +- .../source-control/SourceControlView.test.ts | 10 +- 7 files changed, 188 insertions(+), 45 deletions(-) diff --git a/e2e-tests/provider/suites/source-control-flows.e2e.test.ts b/e2e-tests/provider/suites/source-control-flows.e2e.test.ts index 7a77fa3..7603bd1 100644 --- a/e2e-tests/provider/suites/source-control-flows.e2e.test.ts +++ b/e2e-tests/provider/suites/source-control-flows.e2e.test.ts @@ -615,7 +615,7 @@ describe('Source Control Flows E2E', () => { const deleted = change(deletePath, 'local-deleted'); const { actionService, operations } = s.selectionStack([modified, deleted]); - await actionService.sync([modified.id, deleted.id]); + await actionService.sync([{ changeId: modified.id }, { changeId: deleted.id }]); expect(operations.get(modified.id)).toBe('success'); expect(operations.get(deleted.id)).toBe('success'); @@ -634,7 +634,7 @@ describe('Source Control Flows E2E', () => { const remoteOnly = change(p, 'remote-only'); const { actionService, operations } = s.selectionStack([remoteOnly]); - await actionService.sync([remoteOnly.id]); + await actionService.sync([{ changeId: remoteOnly.id }]); expect(operations.get(remoteOnly.id)).toBe('success'); expect(await s.readLocal(p)).toBe('remote-content'); diff --git a/e2e-tests/provider/support/two-client-sync-scenario.ts b/e2e-tests/provider/support/two-client-sync-scenario.ts index abd9834..2470e54 100644 --- a/e2e-tests/provider/support/two-client-sync-scenario.ts +++ b/e2e-tests/provider/support/two-client-sync-scenario.ts @@ -189,8 +189,8 @@ export class TwoClient { */ async sync(): Promise { await this.refresh(); - const changeIds = this.repository.getAll().map(change => change.id); - await timed(`sync ${this.name}`, () => this.actionService.sync(changeIds)); + const intents = this.repository.getAll().map(change => ({ changeId: change.id })); + await timed(`sync ${this.name}`, () => this.actionService.sync(intents)); } /** Push-only path (the per-row Sync/Push on one or more changes). */ diff --git a/src/logic/source-control/SourceControlActionService.ts b/src/logic/source-control/SourceControlActionService.ts index 9cdb841..f1e8d34 100644 --- a/src/logic/source-control/SourceControlActionService.ts +++ b/src/logic/source-control/SourceControlActionService.ts @@ -2,7 +2,7 @@ import type { PlannedPushBatch } from '../sync/PushCoordinator'; import type { SyncWorkspace } from '../sync/SyncWorkspace'; import { isSyncPlanEmpty, type DeleteQueueEntry, type PushResults, type SyncPlan, type SyncPlanEntry } from '../sync/types'; import { type SyncExecutionResult, type SyncResultNotificationPort } from './SyncResultNotifier'; -import { defaultSyncAction } from './ChangeActionPolicy'; +import { resolveSyncAction, type SyncAction } from './ChangeActionPolicy'; import type { ChangeRepository } from './ChangeRepository'; import type { OperationState } from './OperationState'; import type { SourceControlItem } from './SourceControlViewModel'; @@ -11,6 +11,18 @@ import type { ChangeId, SyncChange } from './types'; /** Which side wins when resolving a change in the 'conflict' state. */ export type ConflictResolution = 'local' | 'remote'; +/** + * One change to sync, with the caller's explicit action choice if it made + * one. `action` omitted (or no longer legal for the change's current kind — + * see {@link resolveSyncAction}) means "use the kind's default", so a stale + * intent from a UI snapshot can never force an action the change can't + * support. + */ +export interface SyncIntentRequest { + changeId: ChangeId; + action?: SyncAction; +} + /** Diff payload the Source Control diff pane can render directly (text-only; binary/symlink changes resolve to `null`). */ export interface SourceControlDiffContent { remote: string; @@ -89,29 +101,31 @@ export class SourceControlActionService { /** * Syncs one or more changes as a single Sync Plan — the Sync Queue - * button's only entry point. Splits `changeIds` by - * {@link defaultSyncAction} into push/delete-remote/pull buckets, plans - * each without mutating anything, merges the result into one `SyncPlan`, - * shows exactly one confirm, and — if confirmed — commits the whole - * remote mutation set (pushes + moves + deletions) through - * `SyncWorkspace.commitResolvedBatch` as one provider call, then applies - * the pull bucket (zero-commit, local-only) separately. This is the fix - * for the "one Sync produces two remote commits" bug: previously the - * Sync Queue routed push/pull/delete-remote through three independent - * `SyncWorkspace` calls, each committing on its own. + * button's only entry point. Splits the requested intents by + * {@link resolveSyncAction} (an explicit per-change action if the caller + * gave one and it's still legal, otherwise the kind's default) into + * push/delete-remote/pull buckets, plans each without mutating anything, + * merges the result into one `SyncPlan`, shows exactly one confirm, and — + * if confirmed — commits the whole remote mutation set (pushes + moves + + * deletions) through `SyncWorkspace.commitResolvedBatch` as one provider + * call, then applies the pull bucket (zero-commit, local-only) + * separately. This is the fix for the "one Sync produces two remote + * commits" bug: previously the Sync Queue routed push/pull/delete-remote + * through three independent `SyncWorkspace` calls, each committing on its + * own. */ - async sync(changeIds: readonly ChangeId[]): Promise { - const targets = this.resolve(changeIds); - if (targets.length === 0) return; + async sync(intents: readonly SyncIntentRequest[]): Promise { + const resolved = this.resolveIntents(intents); + if (resolved.length === 0) return; + const targets = resolved.map(entry => entry.change); const pushTargets: SyncChange[] = []; const deleteTargets: SyncChange[] = []; const pullTargets: SyncChange[] = []; - for (const target of targets) { - const action = defaultSyncAction(target.kind); - if (action === 'pull') pullTargets.push(target); - else if (action === 'delete-remote') deleteTargets.push(target); - else pushTargets.push(target); + for (const { change, action } of resolved) { + if (action === 'pull') pullTargets.push(change); + else if (action === 'delete-remote') deleteTargets.push(change); + else pushTargets.push(change); } let plan: { planned: PlannedPushBatch; confirmed: boolean } | null; @@ -327,6 +341,24 @@ export class SourceControlActionService { return targets; } + /** + * Resolves sync intents to their current `SyncChange` plus the action + * each actually syncs as, dropping any change no longer known to the + * repository. Legality is re-checked here against the change's *current* + * kind (not whatever it was when the caller snapshotted it), via + * {@link resolveSyncAction} — so a stale intent degrades to the default + * instead of ever forcing an action the current kind can't support. + */ + private resolveIntents(intents: readonly SyncIntentRequest[]): Array<{ change: SyncChange; action: SyncAction }> { + const resolved: Array<{ change: SyncChange; action: SyncAction }> = []; + for (const intent of intents) { + const change = this.changes.getById(intent.changeId); + if (!change) continue; + resolved.push({ change, action: resolveSyncAction(change.kind, intent.action) }); + } + return resolved; + } + private startAll(targets: readonly SyncChange[]): void { for (const target of targets) this.operations.start(target.id); } diff --git a/src/ui/source-control/SourceControlView.ts b/src/ui/source-control/SourceControlView.ts index 406cd95..0d8f99b 100644 --- a/src/ui/source-control/SourceControlView.ts +++ b/src/ui/source-control/SourceControlView.ts @@ -2,6 +2,7 @@ import { debounce, Platform, setIcon, setTooltip } from 'obsidian'; import { t } from '../../i18n'; import type { SourceControlFilter } from '../../logic/source-control/SourceControlFilter'; import { SourceControlViewModel, type SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; +import type { SyncIntentRequest } from '../../logic/source-control/SourceControlActionService'; import type { ChangeId } from '../../logic/source-control/types'; import { ICONS } from '../components/icons'; import { renderDiffViewer, currentDiffLayout, rememberDiffLayout, type DiffViewerHandle } from '../components/DiffViewer'; @@ -25,7 +26,7 @@ export interface SourceControlViewCallbacks { * building, single confirm, and single commit all happen behind this * one call (`SourceControlActionService.sync()`). */ - onSync: (changeIds: ChangeId[]) => void | Promise; + onSync: (intents: SyncIntentRequest[]) => void | Promise; /** * Pulls one or more changes — used only by the inline per-row Download * button (a single `remote-only`/`local-deleted` row), not by the Sync @@ -539,7 +540,10 @@ export class SourceControlView { */ private async runSync(queue: readonly SourceControlItem[]): Promise { if (queue.length === 0) return; - await this.callbacks.onSync(queue.map(item => item.id)); + await this.callbacks.onSync(queue.map(item => ({ + changeId: item.id, + action: item.hasActionOverride ? item.syncAction : undefined, + }))); } /** Pulls a single remote-only change into the vault — the inline Download button. */ diff --git a/tests/logic/source-control/SourceControlActionService.test.ts b/tests/logic/source-control/SourceControlActionService.test.ts index 3ae6b23..dcabd59 100644 --- a/tests/logic/source-control/SourceControlActionService.test.ts +++ b/tests/logic/source-control/SourceControlActionService.test.ts @@ -1,13 +1,18 @@ import { describe, expect, it, vi } from 'vitest'; 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 { SourceControlActionService, type SyncIntentRequest } from '../../../src/logic/source-control/SourceControlActionService'; import type { SyncExecutionResult, SyncResultNotificationPort } from '../../../src/logic/source-control/SyncResultNotifier'; import type { PlannedPushBatch } from '../../../src/logic/sync/PushCoordinator'; -import { toChangeId, type SyncChange } from '../../../src/logic/source-control/types'; +import { toChangeId, type ChangeId, type SyncChange } from '../../../src/logic/source-control/types'; import type { SyncWorkspace } from '../../../src/logic/sync/SyncWorkspace'; import type { BatchPushConflict, PushResults, SyncPlan, SyncResult } from '../../../src/logic/sync/types'; +/** Builds plain sync() intents (no explicit action override) from change ids, for tests that don't exercise overrides. */ +function intents(...changeIds: ChangeId[]): SyncIntentRequest[] { + return changeIds.map(changeId => ({ changeId })); +} + const keepRemoteConflict = (path: string, remoteSha = 'reviewed'): BatchPushConflict => ({ path, name: path, @@ -179,7 +184,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await service.sync([toChangeId('c-1'), toChangeId('c-2')]); + await service.sync(intents(toChangeId('c-1'), toChangeId('c-2'))); expect(commitResolvedBatch).toHaveBeenCalledTimes(1); expect(commitResolvedBatch).toHaveBeenCalledWith( @@ -208,7 +213,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await service.sync([toChangeId('c-1')]); + await service.sync(intents(toChangeId('c-1'))); expect(planPush).not.toHaveBeenCalled(); expect(commitResolvedBatch).not.toHaveBeenCalled(); @@ -240,7 +245,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await service.sync([toChangeId('update'), toChangeId('delete'), toChangeId('download')]); + await service.sync(intents(toChangeId('update'), toChangeId('delete'), toChangeId('download'))); expect(commitResolvedBatch).toHaveBeenCalledTimes(1); expect(applyPull).toHaveBeenCalledWith(['remote.md'], { notify: false }); @@ -262,7 +267,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await service.sync([toChangeId('update')]); + await service.sync(intents(toChangeId('update'))); expect(operations.get(toChangeId('update'))).toBe('failed'); expect(notify).toHaveBeenCalledTimes(1); @@ -278,7 +283,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await expect(service.sync([toChangeId('c-1')])).resolves.toBeUndefined(); + await expect(service.sync(intents(toChangeId('c-1')))).resolves.toBeUndefined(); expect(operations.get(toChangeId('c-1'))).toBe('failed'); expect(notify).toHaveBeenCalledTimes(1); @@ -298,7 +303,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await expect(service.sync([toChangeId('c-1')])).resolves.toBeUndefined(); + await expect(service.sync(intents(toChangeId('c-1')))).resolves.toBeUndefined(); expect(operations.get(toChangeId('c-1'))).toBe('failed'); expect(notify).toHaveBeenCalledTimes(1); @@ -323,7 +328,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await service.sync([toChangeId('push'), toChangeId('pull')]); + await service.sync(intents(toChangeId('push'), toChangeId('pull'))); expect(commitResolvedBatch).toHaveBeenCalledTimes(1); expect(operations.get(toChangeId('push'))).toBe('success'); @@ -344,7 +349,7 @@ describe('SourceControlActionService', () => { fakeWorkspace({ planPush, confirmPlan, commitResolvedBatch }), ); - await service.sync([toChangeId('c-1')]); + await service.sync(intents(toChangeId('c-1'))); expect(confirmPlan).toHaveBeenCalledWith(expect.any(Object), 'sync'); expect(commitResolvedBatch).not.toHaveBeenCalled(); @@ -360,7 +365,7 @@ describe('SourceControlActionService', () => { fakeWorkspace({ planPush, confirmPlan, commitResolvedBatch }), ); - await service.sync([toChangeId('c-1')]); + await service.sync(intents(toChangeId('c-1'))); expect(confirmPlan).not.toHaveBeenCalled(); expect(commitResolvedBatch).not.toHaveBeenCalled(); @@ -383,7 +388,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await service.sync([toChangeId('c-1')]); + await service.sync(intents(toChangeId('c-1'))); expect(commitResolvedBatch).toHaveBeenCalledTimes(1); expect(commitResolvedBatch).toHaveBeenCalledWith( @@ -425,7 +430,7 @@ describe('SourceControlActionService', () => { { notify }, ); - await service.sync([toChangeId('c-1'), toChangeId('c-2')]); + await service.sync(intents(toChangeId('c-1'), toChangeId('c-2'))); expect(operations.get(toChangeId('c-1'))).toBe('success'); expect(operations.get(toChangeId('c-2'))).toBe('failed'); @@ -436,6 +441,108 @@ describe('SourceControlActionService', () => { downloaded: 0, })); }); + + describe('explicit action overrides', () => { + it('honors an explicit pull override on a local-modified change (routes to pull, not the push default)', async () => { + const planPush = vi.fn(); + const planPull = vi.fn().mockResolvedValue(emptySyncPlan({ modifications: [{ path: 'a.md', name: 'a.md' }] })); + const applyPull = vi.fn().mockResolvedValue(emptySyncResult({ added: 0, updated: 1, success: 1 })); + const { service, operations } = buildService( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }], + fakeWorkspace({ planPush, planPull, applyPull }), + ); + + await service.sync([{ changeId: toChangeId('c-1'), action: 'pull' }]); + + expect(planPush).not.toHaveBeenCalled(); + expect(applyPull).toHaveBeenCalledWith(['a.md'], { notify: false }); + expect(operations.get(toChangeId('c-1'))).toBe('success'); + }); + + it('honors an explicit push override on a remote-modified change (routes to push, not the pull default)', async () => { + 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: 'local wins', existingSha: 'sha-a' }], + })); + const planPull = vi.fn(); + const commitResolvedBatch = vi.fn().mockResolvedValue(undefined); + const { service, operations } = buildService( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'remote-modified' }], + fakeWorkspace({ planPush, planPull, commitResolvedBatch }), + ); + + await service.sync([{ changeId: toChangeId('c-1'), action: 'push' }]); + + expect(planPull).not.toHaveBeenCalled(); + expect(commitResolvedBatch).toHaveBeenCalledTimes(1); + expect(operations.get(toChangeId('c-1'))).toBe('success'); + }); + + it('honors an explicit pull override on a local-deleted change (restores instead of mirroring the delete)', async () => { + const planPush = vi.fn(); + const planPull = vi.fn().mockResolvedValue(emptySyncPlan({ additions: [{ path: 'gone.md', name: 'gone.md' }] })); + const applyPull = vi.fn().mockResolvedValue(emptySyncResult({ added: 1, success: 1 })); + const commitResolvedBatch = vi.fn().mockResolvedValue(undefined); + const { service, operations } = buildService( + [{ id: toChangeId('c-1'), path: 'gone.md', kind: 'local-deleted' }], + fakeWorkspace({ planPush, planPull, applyPull, commitResolvedBatch }), + ); + + await service.sync([{ changeId: toChangeId('c-1'), action: 'pull' }]); + + expect(commitResolvedBatch).not.toHaveBeenCalled(); + expect(applyPull).toHaveBeenCalledWith(['gone.md'], { notify: false }); + expect(operations.get(toChangeId('c-1'))).toBe('success'); + }); + + it('falls back to the default when the override is no longer legal for the change\'s current kind', async () => { + // Caller's snapshot said local-modified + pull override; by the + // time sync() runs the repository already reports local-only + // (remote copy gone) — 'pull' isn't legal there, so it should + // fall back to push instead of being silently dropped or throwing. + const planPush = vi.fn().mockResolvedValue(emptyPlannedBatch({ + reviewPlan: emptySyncPlan({ additions: [{ path: 'a.md', name: 'a.md' }] }), + pushes: [{ path: 'a.md', name: 'a.md', repoPath: 'a.md', content: 'local', existingSha: undefined }], + })); + const commitResolvedBatch = vi.fn().mockResolvedValue(undefined); + const { service, operations } = buildService( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }], + fakeWorkspace({ planPush, commitResolvedBatch }), + ); + + await service.sync([{ changeId: toChangeId('c-1'), action: 'pull' }]); + + expect(commitResolvedBatch).toHaveBeenCalledTimes(1); + expect(operations.get(toChangeId('c-1'))).toBe('success'); + }); + + it('merges mixed default and overridden intents into one plan and one remote commit', async () => { + const commitResolvedBatch = vi.fn().mockResolvedValue(undefined); + const planPush = vi.fn().mockResolvedValue(emptyPlannedBatch({ + reviewPlan: emptySyncPlan({ modifications: [{ path: 'push-me.md', name: 'push-me.md' }] }), + pushes: [{ path: 'push-me.md', name: 'push-me.md', repoPath: 'push-me.md', content: 'x', existingSha: 'sha' }], + })); + const planPull = vi.fn().mockResolvedValue(emptySyncPlan({ modifications: [{ path: 'pull-me.md', name: 'pull-me.md' }] })); + const applyPull = vi.fn().mockResolvedValue(emptySyncResult({ updated: 1, success: 1 })); + const { service, operations } = buildService( + [ + { id: toChangeId('push-default'), path: 'push-me.md', kind: 'local-modified' }, + { id: toChangeId('pull-override'), path: 'pull-me.md', kind: 'local-modified' }, + ], + fakeWorkspace({ planPush, planPull, applyPull, commitResolvedBatch }), + ); + + await service.sync([ + { changeId: toChangeId('push-default') }, + { changeId: toChangeId('pull-override'), action: 'pull' }, + ]); + + expect(commitResolvedBatch).toHaveBeenCalledTimes(1); + expect(applyPull).toHaveBeenCalledWith(['pull-me.md'], { notify: false }); + expect(operations.get(toChangeId('push-default'))).toBe('success'); + expect(operations.get(toChangeId('pull-override'))).toBe('success'); + }); + }); }); describe('pull', () => { diff --git a/tests/ui/source-control/SourceControlItemView.test.ts b/tests/ui/source-control/SourceControlItemView.test.ts index ccbfb12..a6921f6 100644 --- a/tests/ui/source-control/SourceControlItemView.test.ts +++ b/tests/ui/source-control/SourceControlItemView.test.ts @@ -84,7 +84,7 @@ describe('SourceControlItemView', () => { const container = view.containerEl.children[1] as HTMLElement; (container.querySelector('.scv-push-btn') as HTMLButtonElement).click(); - expect(sync).toHaveBeenCalledWith([toChangeId('a.md')]); + expect(sync).toHaveBeenCalledWith([{ changeId: toChangeId('a.md'), action: undefined }]); }); it('waits for sync to settle through the production runAction wiring before re-rendering (regression: runAction used to discard the action promise)', async () => { @@ -104,7 +104,7 @@ describe('SourceControlItemView', () => { const container = view.containerEl.children[1] as HTMLElement; (container.querySelector('.scv-push-btn') as HTMLButtonElement).click(); - expect(sync).toHaveBeenCalledWith([toChangeId('a.md'), toChangeId('b.md')]); + expect(sync).toHaveBeenCalledWith([{ changeId: toChangeId('a.md'), action: undefined }, { changeId: toChangeId('b.md'), action: undefined }]); resolveSync(); await new Promise(resolve => window.setTimeout(resolve, 0)); diff --git a/tests/ui/source-control/SourceControlView.test.ts b/tests/ui/source-control/SourceControlView.test.ts index b1ad8ca..69e32f2 100644 --- a/tests/ui/source-control/SourceControlView.test.ts +++ b/tests/ui/source-control/SourceControlView.test.ts @@ -434,7 +434,7 @@ describe('SourceControlView', () => { (container.querySelector('.scv-push-btn') as HTMLButtonElement).click(); - expect(onSync).toHaveBeenCalledWith([toChangeId('c-1'), toChangeId('c-2')]); + expect(onSync).toHaveBeenCalledWith([{ changeId: toChangeId('c-1'), action: undefined }, { changeId: toChangeId('c-2'), action: undefined }]); }); it('disables the push button when nothing is selected', () => { @@ -462,7 +462,7 @@ describe('SourceControlView', () => { (container.querySelector('.scv-push-btn') as HTMLButtonElement).click(); expect(onSync).toHaveBeenCalledOnce(); - expect(onSync).toHaveBeenCalledWith([toChangeId('c-1'), toChangeId('c-2')]); + expect(onSync).toHaveBeenCalledWith([{ changeId: toChangeId('c-1'), action: undefined }, { changeId: toChangeId('c-2'), action: undefined }]); }); it('hands a download-only Sync Queue to onSync too', () => { @@ -476,7 +476,7 @@ describe('SourceControlView', () => { (container.querySelector('.scv-push-btn') as HTMLButtonElement).click(); - expect(onSync).toHaveBeenCalledWith([toChangeId('c-1')]); + expect(onSync).toHaveBeenCalledWith([{ changeId: toChangeId('c-1'), action: undefined }]); }); it('hands a local-deleted change in the Sync Queue to onSync, not a separate delete callback', () => { @@ -490,7 +490,7 @@ describe('SourceControlView', () => { (container.querySelector('.scv-push-btn') as HTMLButtonElement).click(); - expect(onSync).toHaveBeenCalledWith([toChangeId('c-1')]); + expect(onSync).toHaveBeenCalledWith([{ changeId: toChangeId('c-1'), action: undefined }]); }); it('shows Upload / Download group labels in the Sync Queue when the queue is mixed', () => { @@ -1360,7 +1360,7 @@ describe('SourceControlView', () => { (container.querySelector('.scv-mobile-sync-btn') as HTMLButtonElement).click(); - expect(onSync).toHaveBeenCalledWith([toChangeId('c-1')]); + expect(onSync).toHaveBeenCalledWith([{ changeId: toChangeId('c-1'), action: undefined }]); }); }); From 8d5d427e3cdb88a308da9f2f7084306a6e6669f3 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 02:39:13 +0000 Subject: [PATCH 09/26] fix(source-control): add compact per-row action control to the Sync Queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sync Queue rows now render a resolved-action control (icon+label on desktop, icon-only on phone) instead of the plain Download button; clicking it opens a Menu scoped to ChangeActionPolicy.availableSyncActions for the row's kind, plus View diff and Remove from Sync Queue. Choosing the kind's own default clears any stored override instead of recording a redundant one. Repository Changes rows are unchanged — the control is Sync Queue-only, per row, not added to every tree/list row. Adds a DOM-backed Menu/MenuItem mock to tests/setup.ts (Obsidian's real Menu drives a native/DOM popover Node can't render) so queue-row menu interactions can be tested the same way as any other rendered control. --- src/i18n/locales/en.ts | 5 + src/i18n/locales/zh-cn.ts | 5 + src/i18n/locales/zh-tw.ts | 5 + src/ui/source-control/ChangeItem.ts | 97 ++++++++++++++++-- src/ui/source-control/SourceControlView.ts | 24 ++++- styles.css | 32 ++++++ tests/setup.ts | 75 ++++++++++++++ .../source-control/SourceControlView.test.ts | 99 +++++++++++++++++++ 8 files changed, 330 insertions(+), 12 deletions(-) diff --git a/src/i18n/locales/en.ts b/src/i18n/locales/en.ts index 975d2c9..6bbf612 100644 --- a/src/i18n/locales/en.ts +++ b/src/i18n/locales/en.ts @@ -212,6 +212,11 @@ const en = { 'sourceControl.queue.delete': 'Delete', 'sourceControl.action.download': 'Download', 'sourceControl.action.download.tooltip': 'Download from remote', + 'sourceControl.queue.action.push': 'Push local', + 'sourceControl.queue.action.pull': 'Use remote', + 'sourceControl.queue.action.deleteRemote': 'Delete remote', + 'sourceControl.queue.menu.viewDiff': 'View diff', + 'sourceControl.queue.menu.removeFromQueue': 'Remove from Sync Queue', 'sourceControl.empty': 'No changes', 'sourceControl.detail.back': 'Back', 'sourceControl.mobile.filesSelected': '{count} files selected', diff --git a/src/i18n/locales/zh-cn.ts b/src/i18n/locales/zh-cn.ts index 6b1ef3f..0bf64c8 100644 --- a/src/i18n/locales/zh-cn.ts +++ b/src/i18n/locales/zh-cn.ts @@ -214,6 +214,11 @@ const zhCn: Partial> = { 'sourceControl.queue.delete': '删除', 'sourceControl.action.download': '下载', 'sourceControl.action.download.tooltip': '从远程下载', + 'sourceControl.queue.action.push': '推送本机', + 'sourceControl.queue.action.pull': '使用远程', + 'sourceControl.queue.action.deleteRemote': '删除远程', + 'sourceControl.queue.menu.viewDiff': '查看差异', + 'sourceControl.queue.menu.removeFromQueue': '从同步队列移除', 'sourceControl.empty': '没有更改', 'sourceControl.detail.back': '返回', 'sourceControl.mobile.filesSelected': '已选 {count} 个文件', diff --git a/src/i18n/locales/zh-tw.ts b/src/i18n/locales/zh-tw.ts index 800c56a..85a0fff 100644 --- a/src/i18n/locales/zh-tw.ts +++ b/src/i18n/locales/zh-tw.ts @@ -214,6 +214,11 @@ const zhTw: Partial> = { 'sourceControl.queue.delete': '刪除', 'sourceControl.action.download': '下載', 'sourceControl.action.download.tooltip': '從遠端下載', + 'sourceControl.queue.action.push': '推送本機', + 'sourceControl.queue.action.pull': '使用遠端', + 'sourceControl.queue.action.deleteRemote': '刪除遠端', + 'sourceControl.queue.menu.viewDiff': '檢視差異', + 'sourceControl.queue.menu.removeFromQueue': '從同步佇列移除', 'sourceControl.empty': '沒有變更', 'sourceControl.detail.back': '返回', 'sourceControl.mobile.filesSelected': '已選 {count} 個檔案', diff --git a/src/ui/source-control/ChangeItem.ts b/src/ui/source-control/ChangeItem.ts index e7d5151..bc55d5e 100644 --- a/src/ui/source-control/ChangeItem.ts +++ b/src/ui/source-control/ChangeItem.ts @@ -1,9 +1,9 @@ -import { setIcon, setTooltip } from 'obsidian'; +import { Menu, setIcon, setTooltip } from 'obsidian'; import { t } from '../../i18n'; import { ICONS } from '../components/icons'; import { renderOperationIndicator } from './OperationIndicator'; import { presentChange, type ChangeStat } from './ChangePresentation'; -import { canDownload } from '../../logic/source-control/ChangeActionPolicy'; +import { availableSyncActions, canDownload, type SyncAction } from '../../logic/source-control/ChangeActionPolicy'; import type { SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; import type { ChangeId } from '../../logic/source-control/types'; @@ -18,6 +18,13 @@ export interface ChangeItemCallbacks { * kinds, so the callback never has to re-classify. */ onDownload?: (item: SourceControlItem) => void; + /** + * Records the user's explicit action choice for a queued change (e.g. + * "Use remote" instead of the default push). Only wired for Sync Queue + * rows — see {@link ChangeItemOptions.showActionControl} — since + * Repository Changes rows don't carry a queue-scoped action to override. + */ + onChangeSyncAction?: (item: SourceControlItem, action: SyncAction) => void; /** Looks up a cached diff stat for a row, if one has been computed. */ getDiffStat?: (id: ChangeId) => ChangeStat | undefined; } @@ -36,6 +43,13 @@ export interface ChangeItemOptions { * to fill the row, leaving room for {@link folderPath} on the right. */ listMode?: boolean; + /** + * Renders the compact action control (resolved action + menu to + * override/view diff/remove) instead of the plain inline Download + * button. Sync Queue rows only — Repository Changes rows stay + * unselector'd per row, matching the pre-existing layout. + */ + showActionControl?: boolean; } /** @@ -88,13 +102,19 @@ export function renderChangeItem( renderDiffStat(row, callbacks.getDiffStat?.(item.id)); - // A change with something to pull from remote (remote-only: add it - // locally; remote-modified: overwrite the local copy; local-deleted: - // restore it locally) carries a direct Download action so the user can - // pull it without first adding it to the Sync Queue. The button stops - // propagation so clicking it doesn't also trigger the row's - // open-diff/open-remote behavior. - if (canDownload(item.kind) && callbacks.onDownload) { + if (options.showActionControl && callbacks.onChangeSyncAction) { + // Sync Queue row: the resolved action plus a menu to override it, + // view the diff, or drop the row from the queue — supersedes the + // plain Download button below (its "use remote" case is one of the + // menu's options), so only one action affordance renders per row. + renderActionControl(row, item, callbacks); + } else if (canDownload(item.kind) && callbacks.onDownload) { + // A change with something to pull from remote (remote-only: add it + // locally; remote-modified: overwrite the local copy; local-deleted: + // restore it locally) carries a direct Download action so the user + // can pull it without first adding it to the Sync Queue. The button + // stops propagation so clicking it doesn't also trigger the row's + // open-diff/open-remote behavior. renderDownloadAction(row, item, callbacks.onDownload); } @@ -124,6 +144,65 @@ function renderDownloadAction(row: HTMLElement, item: SourceControlItem, onDownl btn.addEventListener('click', (evt) => { evt.stopPropagation(); onDownload(item); }); } +/** Icon + label for each {@link SyncAction}, shared by the queue action control and its menu. */ +function actionIcon(action: SyncAction): string { + if (action === 'pull') return ICONS.pull; + if (action === 'delete-remote') return ICONS.delete; + return ICONS.push; +} + +function actionLabel(action: SyncAction): string { + if (action === 'pull') return t('sourceControl.queue.action.pull'); + if (action === 'delete-remote') return t('sourceControl.queue.action.deleteRemote'); + return t('sourceControl.queue.action.push'); +} + +/** + * The compact Sync Queue row action: shows the resolved action (icon + + * label on desktop, icon-only on phone — matching the row's own space + * constraints) and opens a menu to override it, view the diff, or remove the + * row from the queue. Only the actions {@link availableSyncActions} allows + * for the row's kind appear as choices, so the menu can never offer an + * illegal override. + */ +function renderActionControl(row: HTMLElement, item: SourceControlItem, callbacks: ChangeItemCallbacks): void { + const btn = row.createEl('button', { + cls: 'scv-change-action', + attr: { type: 'button' }, + }); + setIcon(btn.createSpan({ cls: 'scv-change-action-icon' }), actionIcon(item.syncAction)); + btn.createSpan({ cls: 'scv-change-action-label', text: actionLabel(item.syncAction) }); + setTooltip(btn, actionLabel(item.syncAction)); + + btn.addEventListener('click', (evt) => { + evt.stopPropagation(); + const menu = new Menu(); + for (const action of availableSyncActions(item.kind)) { + menu.addItem((menuItem) => { + menuItem + .setTitle(actionLabel(action)) + .setIcon(actionIcon(action)) + .setChecked(action === item.syncAction) + .onClick(() => callbacks.onChangeSyncAction?.(item, action)); + }); + } + menu.addSeparator(); + menu.addItem((menuItem) => { + menuItem + .setTitle(t('sourceControl.queue.menu.viewDiff')) + .setIcon(ICONS.diff) + .onClick(() => callbacks.onOpenDiff(item)); + }); + menu.addItem((menuItem) => { + menuItem + .setTitle(t('sourceControl.queue.menu.removeFromQueue')) + .setIcon(ICONS.clear) + .onClick(() => callbacks.onToggleSelect(item.id, false)); + }); + menu.showAtMouseEvent(evt); + }); +} + /** * Renders the +/- diff stat as two colored spans (green additions, red * deletions) so the magnitude and direction read at a glance. Nothing is diff --git a/src/ui/source-control/SourceControlView.ts b/src/ui/source-control/SourceControlView.ts index 0d8f99b..7fb6ff5 100644 --- a/src/ui/source-control/SourceControlView.ts +++ b/src/ui/source-control/SourceControlView.ts @@ -3,6 +3,7 @@ import { t } from '../../i18n'; import type { SourceControlFilter } from '../../logic/source-control/SourceControlFilter'; import { SourceControlViewModel, type SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; import type { SyncIntentRequest } from '../../logic/source-control/SourceControlActionService'; +import { defaultSyncAction, type SyncAction } from '../../logic/source-control/ChangeActionPolicy'; import type { ChangeId } from '../../logic/source-control/types'; import { ICONS } from '../components/icons'; import { renderDiffViewer, currentDiffLayout, rememberDiffLayout, type DiffViewerHandle } from '../components/DiffViewer'; @@ -287,6 +288,7 @@ export class SourceControlView { onToggleFolderSelect: (ids, selected) => this.toggleFolderSelect(ids, selected), onOpenDiff: (item) => this.openDiff(item), onDownload: (item) => this.download(item), + onChangeSyncAction: (item, action) => this.changeSyncAction(item, action), getDiffStat: (id) => this.diffStat.get(id), }; @@ -517,11 +519,11 @@ export class SourceControlView { const groupCount = [upload, download, deleteRemote].filter(group => group.length > 0).length; const mixed = groupCount > 1; if (mixed && upload.length > 0) list.createDiv({ cls: 'scv-queue-group-label', text: t('sourceControl.queue.upload') }); - for (const item of upload) renderChangeItem(list, item, basename(item.path), callbacks); + for (const item of upload) renderChangeItem(list, item, basename(item.path), callbacks, { showActionControl: true }); if (mixed && download.length > 0) list.createDiv({ cls: 'scv-queue-group-label', text: t('sourceControl.queue.download') }); - for (const item of download) renderChangeItem(list, item, basename(item.path), callbacks); + for (const item of download) renderChangeItem(list, item, basename(item.path), callbacks, { showActionControl: true }); if (mixed && deleteRemote.length > 0) list.createDiv({ cls: 'scv-queue-group-label', text: t('sourceControl.queue.delete') }); - for (const item of deleteRemote) renderChangeItem(list, item, basename(item.path), callbacks); + for (const item of deleteRemote) renderChangeItem(list, item, basename(item.path), callbacks, { showActionControl: true }); } /** Unselects every change currently in the Sync Queue in one shot. */ @@ -551,6 +553,22 @@ export class SourceControlView { if (this.callbacks.onPull) void this.callbacks.onPull([item.id]); } + /** + * Records (or clears) a Sync Queue row's explicit action override, chosen + * from its {@link ChangeItemCallbacks.onChangeSyncAction} menu. Picking + * the kind's own default clears the override rather than storing a + * redundant one, so `hasActionOverride` only ever means "the user chose + * something other than the default". + */ + private changeSyncAction(item: SourceControlItem, action: SyncAction): void { + if (action === defaultSyncAction(item.kind)) { + this.viewModel.selection.clearActionOverride(item.id); + } else { + this.viewModel.selection.setActionOverride(item.id, action); + } + this.rerender(); + } + private renderDetail(root: HTMLElement): void { const detail = root.createDiv({ cls: 'scv-detail' }); const bar = detail.createDiv({ cls: 'scv-detail-bar' }); diff --git a/styles.css b/styles.css index 05ca581..22bc334 100644 --- a/styles.css +++ b/styles.css @@ -672,6 +672,38 @@ body.is-mobile .scv-view-toggle-label { display: none; } .scv-change-download-label { white-space: nowrap; } +/* ── Sync Queue row action control (resolved action + override menu) ── */ +.scv-change-action { + display: inline-flex; + align-items: center; + gap: 3px; + margin-left: auto; + padding: 1px 6px; + background: transparent; + border: 1px solid var(--background-modifier-border); + border-radius: var(--radius-s); + color: var(--text-muted); + font-size: 0.72em; + line-height: 1.4; + cursor: pointer; + flex-shrink: 0; +} + +.scv-change-action:hover { + background: var(--background-modifier-hover); + color: var(--text-normal); + border-color: var(--interactive-accent); +} + +.scv-change-action .svg-icon, +.scv-change-action-icon .svg-icon { width: 12px; height: 12px; } + +.scv-change-action-label { white-space: nowrap; } + +/* Phone: icon-only, matching the row's tighter width budget. */ +body.is-mobile .scv-change-action-label { display: none; } +body.is-mobile .scv-change-action { padding: 4px; min-width: 28px; min-height: 28px; justify-content: center; } + .scv-change-rename-from { color: var(--text-faint); text-decoration: line-through; diff --git a/tests/setup.ts b/tests/setup.ts index dd850fe..e846f4b 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -302,6 +302,79 @@ export const FileSystemAdapter = class { getBasePath() { return '/mock/path'; } }; +// Real MenuItem builds a DOM row inside its owning Menu's popover; mirrored +// here (rather than a plain recorder) so tests can query/click menu items +// the same way they interact with any other rendered control. +export class MenuItem { + el: HTMLElement; + titleEl: HTMLElement; + private clickCb?: (evt: MouseEvent | KeyboardEvent) => void; + + constructor() { + this.el = document.createElement('div'); + this.el.className = 'menu-item'; + this.titleEl = document.createElement('div'); + this.titleEl.className = 'menu-item-title'; + this.el.appendChild(this.titleEl); + this.el.addEventListener('click', (evt) => this.clickCb?.(evt)); + } + + setTitle(title: string) { + this.titleEl.textContent = title; + this.el.setAttribute('data-title', title); + return this; + } + + setIcon(icon: string | null) { + if (icon) this.el.setAttribute('data-icon', icon); + return this; + } + + setChecked(checked: boolean | null) { + this.el.setAttribute('data-checked', String(checked)); + this.el.classList.toggle('is-checked', checked === true); + return this; + } + + onClick(cb: (evt: MouseEvent | KeyboardEvent) => unknown) { + this.clickCb = cb; + return this; + } +} + +// Real Menu shows a native/DOM popover on showAtMouseEvent/showAtPosition; +// mocked here as a plain element appended to document.body so tests can find +// and click its items instead of reaching into internal state. +export class Menu { + menuEl: HTMLElement; + + constructor() { + this.menuEl = document.createElement('div'); + this.menuEl.className = 'menu'; + } + + addItem(cb: (item: MenuItem) => unknown) { + const item = new MenuItem(); + cb(item); + this.menuEl.appendChild(item.el); + return this; + } + + addSeparator() { + this.menuEl.appendChild(Object.assign(document.createElement('div'), { className: 'menu-separator' })); + return this; + } + + setNoIcon() { return this; } + setUseNativeMenu() { return this; } + setParentElement() { return this; } + showAtMouseEvent() { document.body.appendChild(this.menuEl); return this; } + showAtPosition() { document.body.appendChild(this.menuEl); return this; } + hide() { this.menuEl.remove(); return this; } + close() { this.menuEl.remove(); } + onHide() {} +} + vi.mock('obsidian', () => ({ Plugin, PluginSettingTab, @@ -326,4 +399,6 @@ vi.mock('obsidian', () => ({ setIcon, Platform, FileSystemAdapter, + Menu, + MenuItem, })); diff --git a/tests/ui/source-control/SourceControlView.test.ts b/tests/ui/source-control/SourceControlView.test.ts index 69e32f2..69dfeae 100644 --- a/tests/ui/source-control/SourceControlView.test.ts +++ b/tests/ui/source-control/SourceControlView.test.ts @@ -541,6 +541,105 @@ describe('SourceControlView', () => { }); }); + describe('queue row action control', () => { + // Menu popovers append to document.body (outside `container`), so + // each test's menu must be cleared before the next opens one. + afterEach(() => { document.querySelectorAll('.menu').forEach(el => el.remove()); }); + + it('renders the action control on a Sync Queue row but not on a Repository Changes row', () => { + const { view, selection } = buildView([ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }, + { id: toChangeId('c-2'), path: 'b.md', kind: 'local-modified' }, + ]); + selection.selectForSync(toChangeId('c-1')); + view.render(container); + + const queueSection = container.querySelector('.scv-selected-section') as HTMLElement; + const treeSection = container.querySelector('.scv-changes-tree') as HTMLElement; + expect(queueSection.querySelector('.scv-change-action')).toBeTruthy(); + expect(treeSection.querySelector('.scv-change-action')).toBeNull(); + }); + + it('opening the menu offers only the actions legal for the row\'s kind, checking the resolved one', () => { + const { view, selection } = buildView([ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }, + ]); + selection.selectForSync(toChangeId('c-1')); + view.render(container); + + const btn = container.querySelector('.scv-selected-section .scv-change-action') as HTMLButtonElement; + btn.click(); + + const items = Array.from(document.querySelectorAll('.menu .menu-item')); + expect(items.map(el => el.getAttribute('data-title'))).toEqual( + expect.arrayContaining(['Push local', 'Use remote', 'View diff', 'Remove from Sync Queue']), + ); + const pushItem = items.find(el => el.getAttribute('data-title') === 'Push local'); + expect(pushItem?.getAttribute('data-checked')).toBe('true'); + // delete-remote isn't legal for local-modified, so it must not appear. + expect(items.some(el => el.getAttribute('data-title') === 'Delete remote')).toBe(false); + }); + + it('choosing "Use remote" from the menu sets an override that survives to the queue grouping', () => { + const { view, selection } = buildView([ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }, + ]); + selection.selectForSync(toChangeId('c-1')); + view.render(container); + + (container.querySelector('.scv-selected-section .scv-change-action') as HTMLButtonElement).click(); + const useRemote = Array.from(document.querySelectorAll('.menu .menu-item')) + .find(el => el.getAttribute('data-title') === 'Use remote') as HTMLElement; + useRemote.click(); + + expect(selection.getActionOverride(toChangeId('c-1'))).toBe('pull'); + }); + + it('choosing "Remove from Sync Queue" deselects the row', () => { + const { view, selection } = buildView([ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }, + ]); + selection.selectForSync(toChangeId('c-1')); + view.render(container); + + (container.querySelector('.scv-selected-section .scv-change-action') as HTMLButtonElement).click(); + const remove = Array.from(document.querySelectorAll('.menu .menu-item')) + .find(el => el.getAttribute('data-title') === 'Remove from Sync Queue') as HTMLElement; + remove.click(); + + expect(selection.isIncluded(toChangeId('c-1'))).toBe(false); + }); + + it('choosing "View diff" from the menu opens the diff instead of changing the action', () => { + const onOpenDiff = vi.fn(); + const { view, selection } = buildView( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }], + { onOpenDiff }, + ); + selection.selectForSync(toChangeId('c-1')); + view.render(container); + + (container.querySelector('.scv-selected-section .scv-change-action') as HTMLButtonElement).click(); + const viewDiff = Array.from(document.querySelectorAll('.menu .menu-item')) + .find(el => el.getAttribute('data-title') === 'View diff') as HTMLElement; + viewDiff.click(); + + expect(onOpenDiff).toHaveBeenCalledWith(expect.objectContaining({ id: toChangeId('c-1') })); + }); + + it('does not render a plain Download button on a Sync Queue row (the action control supersedes it)', () => { + const { view, selection } = buildView([ + { id: toChangeId('c-1'), path: 'remote.md', kind: 'remote-only' }, + ]); + selection.selectForSync(toChangeId('c-1')); + view.render(container); + + const queueSection = container.querySelector('.scv-selected-section') as HTMLElement; + expect(queueSection.querySelector('.scv-change-download')).toBeNull(); + expect(queueSection.querySelector('.scv-change-action')).toBeTruthy(); + }); + }); + describe('inline download action', () => { it('renders a Download button on a remote-only tree row and routes it to onPull', () => { const onPull = vi.fn(); From b44389d1eb6afc5f17d77ef4d42724003461b713 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 02:40:30 +0000 Subject: [PATCH 10/26] fix(source-control): shrink the inline Download button to icon-only on phone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Repository Changes row's Download button still rendered its full text label on phone, crowding the filename at narrow widths — the mobile-diff branch fixed diff rendering but never touched this control. Presentation-only: canDownload()/onDownload() are unchanged, the label stays in the DOM (screen readers/tooltip still get it), only its visual display and the button's padding change under body.is-mobile. --- styles.css | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/styles.css b/styles.css index 22bc334..028dd73 100644 --- a/styles.css +++ b/styles.css @@ -672,6 +672,10 @@ body.is-mobile .scv-view-toggle-label { display: none; } .scv-change-download-label { white-space: nowrap; } +/* Phone: icon-only, so the button doesn't crowd out the filename at narrow widths. */ +body.is-mobile .scv-change-download-label { display: none; } +body.is-mobile .scv-change-download { padding: 4px; min-width: 28px; min-height: 28px; justify-content: center; } + /* ── Sync Queue row action control (resolved action + override menu) ── */ .scv-change-action { display: inline-flex; From 1a54acba09e6a20f25e2764e0c414903f25b0b99 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 02:54:31 +0000 Subject: [PATCH 11/26] fix(source-control): add per-kind advanced action menu to Repository Changes rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a "⋯" row menu (rowMenuActions in ChangeItem.ts) offering the immediate actions relevant to a change's kind — e.g. local-modified gets Push local / Use remote… / View diff / Add to Sync Queue / Delete local…, remote-only gets Download / Delete remote… / Add to Sync Queue / Open remote. conflict/synced get no menu (conflict resolution keeps its own dedicated UI; synced is never actionable). Wired through new onPush/ onDeleteRemote/onDeleteLocal callbacks on SourceControlViewCallbacks, executed immediately via SourceControlActionService — distinct from queuing, which stays override-based via the existing action control. Delete remote goes through the existing ConfirmModal first (no equivalent safety net to Obsidian's own trash, which deleteLocal already uses); delete local does not re-confirm since trashFile already is one. The Modal mock in tests/setup.ts now appends modalEl to document.body on open() (matching real Obsidian), needed to test the confirm flow the same way other rendered controls are tested. --- src/i18n/locales/en.ts | 13 ++ src/i18n/locales/zh-cn.ts | 13 ++ src/i18n/locales/zh-tw.ts | 13 ++ src/ui/components/icons.ts | 4 + src/ui/source-control/ChangeItem.ts | 122 +++++++++++++++++- .../source-control/SourceControlItemView.ts | 30 ++++- src/ui/source-control/SourceControlView.ts | 36 +++++- styles.css | 24 ++++ tests/setup.ts | 5 + .../SourceControlItemView.test.ts | 50 ++++++- .../source-control/SourceControlView.test.ts | 106 +++++++++++++++ 11 files changed, 409 insertions(+), 7 deletions(-) diff --git a/src/i18n/locales/en.ts b/src/i18n/locales/en.ts index 6bbf612..99b2bb5 100644 --- a/src/i18n/locales/en.ts +++ b/src/i18n/locales/en.ts @@ -217,6 +217,19 @@ const en = { 'sourceControl.queue.action.deleteRemote': 'Delete remote', 'sourceControl.queue.menu.viewDiff': 'View diff', 'sourceControl.queue.menu.removeFromQueue': 'Remove from Sync Queue', + 'sourceControl.row.menu.tooltip': 'More actions', + 'sourceControl.row.menu.pushLocal': 'Push local', + 'sourceControl.row.menu.useRemote': 'Use remote', + 'sourceControl.row.menu.useRemoteEllipsis': 'Use remote…', + 'sourceControl.row.menu.pushLocalEllipsis': 'Push local…', + 'sourceControl.row.menu.deleteRemote': 'Delete remote', + 'sourceControl.row.menu.deleteRemoteEllipsis': 'Delete remote…', + 'sourceControl.row.menu.deleteLocalEllipsis': 'Delete local…', + 'sourceControl.row.menu.restoreLocal': 'Restore local', + 'sourceControl.row.menu.viewDiff': 'View diff', + 'sourceControl.row.menu.addToQueue': 'Add to Sync Queue', + 'sourceControl.row.menu.openRemote': 'Open remote', + 'sourceControl.row.confirmDeleteRemote': 'Delete "{path}" from the remote? This does not affect the local copy.', 'sourceControl.empty': 'No changes', 'sourceControl.detail.back': 'Back', 'sourceControl.mobile.filesSelected': '{count} files selected', diff --git a/src/i18n/locales/zh-cn.ts b/src/i18n/locales/zh-cn.ts index 0bf64c8..7c6e810 100644 --- a/src/i18n/locales/zh-cn.ts +++ b/src/i18n/locales/zh-cn.ts @@ -219,6 +219,19 @@ const zhCn: Partial> = { 'sourceControl.queue.action.deleteRemote': '删除远程', 'sourceControl.queue.menu.viewDiff': '查看差异', 'sourceControl.queue.menu.removeFromQueue': '从同步队列移除', + 'sourceControl.row.menu.tooltip': '更多操作', + 'sourceControl.row.menu.pushLocal': '推送本机', + 'sourceControl.row.menu.useRemote': '使用远程', + 'sourceControl.row.menu.useRemoteEllipsis': '使用远程…', + 'sourceControl.row.menu.pushLocalEllipsis': '推送本机…', + 'sourceControl.row.menu.deleteRemote': '删除远程', + 'sourceControl.row.menu.deleteRemoteEllipsis': '删除远程…', + 'sourceControl.row.menu.deleteLocalEllipsis': '删除本机…', + 'sourceControl.row.menu.restoreLocal': '还原本机', + 'sourceControl.row.menu.viewDiff': '查看差异', + 'sourceControl.row.menu.addToQueue': '加入同步队列', + 'sourceControl.row.menu.openRemote': '打开远程', + 'sourceControl.row.confirmDeleteRemote': '要从远程删除“{path}”吗?这不会影响本地副本。', 'sourceControl.empty': '没有更改', 'sourceControl.detail.back': '返回', 'sourceControl.mobile.filesSelected': '已选 {count} 个文件', diff --git a/src/i18n/locales/zh-tw.ts b/src/i18n/locales/zh-tw.ts index 85a0fff..d43b7f6 100644 --- a/src/i18n/locales/zh-tw.ts +++ b/src/i18n/locales/zh-tw.ts @@ -219,6 +219,19 @@ const zhTw: Partial> = { 'sourceControl.queue.action.deleteRemote': '刪除遠端', 'sourceControl.queue.menu.viewDiff': '檢視差異', 'sourceControl.queue.menu.removeFromQueue': '從同步佇列移除', + 'sourceControl.row.menu.tooltip': '更多操作', + 'sourceControl.row.menu.pushLocal': '推送本機', + 'sourceControl.row.menu.useRemote': '使用遠端', + 'sourceControl.row.menu.useRemoteEllipsis': '使用遠端…', + 'sourceControl.row.menu.pushLocalEllipsis': '推送本機…', + 'sourceControl.row.menu.deleteRemote': '刪除遠端', + 'sourceControl.row.menu.deleteRemoteEllipsis': '刪除遠端…', + 'sourceControl.row.menu.deleteLocalEllipsis': '刪除本機…', + 'sourceControl.row.menu.restoreLocal': '還原本機', + 'sourceControl.row.menu.viewDiff': '檢視差異', + 'sourceControl.row.menu.addToQueue': '加入同步佇列', + 'sourceControl.row.menu.openRemote': '開啟遠端', + 'sourceControl.row.confirmDeleteRemote': '要從遠端刪除「{path}」嗎?這不會影響本機副本。', 'sourceControl.empty': '沒有變更', 'sourceControl.detail.back': '返回', 'sourceControl.mobile.filesSelected': '已選 {count} 個檔案', diff --git a/src/ui/components/icons.ts b/src/ui/components/icons.ts index e879edb..33bd8c7 100644 --- a/src/ui/components/icons.ts +++ b/src/ui/components/icons.ts @@ -35,4 +35,8 @@ export const ICONS = { // Repository changes view toggle viewTree: 'folder-tree', viewList: 'list', + // Row action menu (Repository Changes "⋯") + rowMenu: 'more-horizontal', + addToQueue: 'list-plus', + openRemote: 'external-link', } as const; diff --git a/src/ui/source-control/ChangeItem.ts b/src/ui/source-control/ChangeItem.ts index bc55d5e..a756c8d 100644 --- a/src/ui/source-control/ChangeItem.ts +++ b/src/ui/source-control/ChangeItem.ts @@ -1,11 +1,14 @@ import { Menu, setIcon, setTooltip } from 'obsidian'; -import { t } from '../../i18n'; +import { t, type TranslationKey } from '../../i18n'; import { ICONS } from '../components/icons'; import { renderOperationIndicator } from './OperationIndicator'; import { presentChange, type ChangeStat } from './ChangePresentation'; import { availableSyncActions, canDownload, type SyncAction } from '../../logic/source-control/ChangeActionPolicy'; import type { SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; -import type { ChangeId } from '../../logic/source-control/types'; +import type { ChangeId, SyncChangeKind } from '../../logic/source-control/types'; + +/** An immediate (not queued) row action, invoked from the Repository Changes "⋯" menu. */ +export type RowActionKind = 'push' | 'pull' | 'delete-remote' | 'delete-local'; export interface ChangeItemCallbacks { onToggleSelect: (id: ChangeId, selected: boolean) => void; @@ -25,6 +28,16 @@ export interface ChangeItemCallbacks { * Repository Changes rows don't carry a queue-scoped action to override. */ onChangeSyncAction?: (item: SourceControlItem, action: SyncAction) => void; + /** + * Runs an immediate push/pull/delete-remote/delete-local on a single + * Repository Changes row, from its "⋯" menu — distinct from queueing: + * this executes right away rather than waiting for the next Sync. Only + * rendered for kinds {@link rowMenuActions} has entries for; the row's + * own kind decides which `RowActionKind`s are actually offered. + */ + onRowAction?: (item: SourceControlItem, action: RowActionKind) => void; + /** Opens the change's file on the remote (in the browser) from the row menu — same destination as clicking a `remote-only` row. */ + onOpenRemote?: (item: SourceControlItem) => void; /** Looks up a cached diff stat for a row, if one has been computed. */ getDiffStat?: (id: ChangeId) => ChangeStat | undefined; } @@ -118,6 +131,10 @@ export function renderChangeItem( renderDownloadAction(row, item, callbacks.onDownload); } + if (!options.showActionControl && callbacks.onRowAction) { + renderRowMenuButton(row, item, callbacks); + } + renderOperationIndicator(row, item.operationStatus); row.addEventListener('click', (evt) => { @@ -203,6 +220,107 @@ function renderActionControl(row: HTMLElement, item: SourceControlItem, callback }); } +/** One entry in a row's "⋯" menu: either an immediate {@link RowActionKind} or one of the fixed extras (view diff, queue, open remote). */ +type RowMenuEntry = + | { kind: 'action'; action: RowActionKind; labelKey: TranslationKey; icon: string } + | { kind: 'view-diff' } + | { kind: 'add-to-queue' } + | { kind: 'open-remote' }; + +/** + * The Repository Changes row menu's contents, per change kind. Deliberately + * hand-written per kind rather than derived from {@link availableSyncActions} + * — that table describes what the *Sync Queue* may resolve a change to, + * which isn't the same set as what's useful to run *immediately* from a + * single row (e.g. `local-only` has no remote counterpart to diff against, + * so it gets no "View diff" entry; `conflict` is excluded entirely since its + * resolution already has a dedicated UI — see the "conflict path" note on + * `SourceControlActionService`). + */ +function rowMenuActions(kind: SyncChangeKind): readonly RowMenuEntry[] { + switch (kind) { + case 'local-only': + return [ + { kind: 'action', action: 'push', labelKey: 'sourceControl.row.menu.pushLocal', icon: ICONS.push }, + { kind: 'add-to-queue' }, + { kind: 'action', action: 'delete-local', labelKey: 'sourceControl.row.menu.deleteLocalEllipsis', icon: ICONS.delete }, + ]; + case 'local-modified': + return [ + { kind: 'action', action: 'push', labelKey: 'sourceControl.row.menu.pushLocal', icon: ICONS.push }, + { kind: 'action', action: 'pull', labelKey: 'sourceControl.row.menu.useRemoteEllipsis', icon: ICONS.pull }, + { kind: 'view-diff' }, + { kind: 'add-to-queue' }, + { kind: 'action', action: 'delete-local', labelKey: 'sourceControl.row.menu.deleteLocalEllipsis', icon: ICONS.delete }, + ]; + case 'remote-modified': + return [ + { kind: 'action', action: 'pull', labelKey: 'sourceControl.row.menu.useRemote', icon: ICONS.pull }, + { kind: 'action', action: 'push', labelKey: 'sourceControl.row.menu.pushLocalEllipsis', icon: ICONS.push }, + { kind: 'view-diff' }, + { kind: 'add-to-queue' }, + ]; + case 'remote-only': + return [ + { kind: 'action', action: 'pull', labelKey: 'sourceControl.action.download', icon: ICONS.download }, + { kind: 'action', action: 'delete-remote', labelKey: 'sourceControl.row.menu.deleteRemoteEllipsis', icon: ICONS.delete }, + { kind: 'add-to-queue' }, + { kind: 'open-remote' }, + ]; + case 'local-deleted': + return [ + { kind: 'action', action: 'delete-remote', labelKey: 'sourceControl.row.menu.deleteRemote', icon: ICONS.delete }, + { kind: 'action', action: 'pull', labelKey: 'sourceControl.row.menu.restoreLocal', icon: ICONS.pull }, + ]; + case 'moved': + return [ + { kind: 'action', action: 'push', labelKey: 'sourceControl.row.menu.pushLocal', icon: ICONS.push }, + { kind: 'add-to-queue' }, + ]; + case 'conflict': + case 'synced': + return []; + } +} + +/** + * The Repository Changes row's "⋯" menu button: immediate push/pull/delete + * actions plus view-diff/queue/open-remote, scoped per kind by + * {@link rowMenuActions}. Rendered only when there's at least one entry for + * the row's kind (`conflict`/`synced` get none, so no empty menu appears). + */ +function renderRowMenuButton(row: HTMLElement, item: SourceControlItem, callbacks: ChangeItemCallbacks): void { + const entries = rowMenuActions(item.kind); + if (entries.length === 0) return; + + const btn = row.createEl('button', { cls: 'scv-change-menu', attr: { type: 'button' } }); + setIcon(btn, ICONS.rowMenu); + setTooltip(btn, t('sourceControl.row.menu.tooltip')); + + btn.addEventListener('click', (evt) => { + evt.stopPropagation(); + const menu = new Menu(); + for (const entry of entries) { + menu.addItem((menuItem) => { + if (entry.kind === 'action') { + menuItem.setTitle(t(entry.labelKey)).setIcon(entry.icon) + .onClick(() => callbacks.onRowAction?.(item, entry.action)); + } else if (entry.kind === 'view-diff') { + menuItem.setTitle(t('sourceControl.row.menu.viewDiff')).setIcon(ICONS.diff) + .onClick(() => callbacks.onOpenDiff(item)); + } else if (entry.kind === 'add-to-queue') { + menuItem.setTitle(t('sourceControl.row.menu.addToQueue')).setIcon(ICONS.addToQueue) + .onClick(() => callbacks.onToggleSelect(item.id, true)); + } else { + menuItem.setTitle(t('sourceControl.row.menu.openRemote')).setIcon(ICONS.openRemote) + .onClick(() => callbacks.onOpenRemote?.(item)); + } + }); + } + menu.showAtMouseEvent(evt); + }); +} + /** * Renders the +/- diff stat as two colored spans (green additions, red * deletions) so the magnitude and direction read at a glance. Nothing is diff --git a/src/ui/source-control/SourceControlItemView.ts b/src/ui/source-control/SourceControlItemView.ts index 0842bd8..5297542 100644 --- a/src/ui/source-control/SourceControlItemView.ts +++ b/src/ui/source-control/SourceControlItemView.ts @@ -4,11 +4,12 @@ import { t } from '../../i18n'; import type { SourceControlItem } from '../../logic/source-control/SourceControlViewModel'; import { resolveSyncAction } from '../../logic/source-control/ChangeActionPolicy'; import type { FileStatus } from '../../logic/sync-status-service'; -import { toChangeId } from '../../logic/source-control/types'; +import { toChangeId, type ChangeId } from '../../logic/source-control/types'; import { SourceControlView, type SourceControlViewCallbacks } from './SourceControlView'; import type { SourceControlWorkspaceInfo } from './SourceControlHeader'; import { addedContentStat, cheapLocalStat, computeDiffStat, deletedContentStat } from './ChangePresentation'; import type { DiffStatLoadResult } from './DiffStatProvider'; +import { ConfirmModal } from '../ConfirmModal'; // Reuses the legacy sync-status view's registered type string so an already // open/pinned leaf from before this cutover resolves into the new view @@ -59,6 +60,9 @@ export class SourceControlItemView extends ItemView { onOpenDiff: (item) => { if (!Platform.isMobile) void this.openDesktopDiffTab(item); }, onOpenLocalFile: (item) => this.openLocalFile(item.path), onOpenRemoteFile: (item) => this.openRemoteFile(item.path), + onPush: (changeIds) => this.runAction(this.plugin.sourceControlActions.push(changeIds)), + onDeleteRemote: (changeIds) => this.runAction(this.confirmThenDeleteRemote(changeIds)), + onDeleteLocal: (changeIds) => this.runAction(this.plugin.sourceControlActions.deleteLocal(changeIds)), }; this.view = new SourceControlView( this.plugin.sourceControlViewModel, @@ -152,6 +156,30 @@ export class SourceControlItemView extends ItemView { if (url) window.open(url, '_blank'); } + /** + * Confirms before running the row menu's one-off "Delete remote" — unlike + * `deleteLocal` (which goes through Obsidian's own trash), a remote + * deletion outside the Sync Plan's own confirm has no equivalent safety + * net, so this reuses the same `ConfirmModal` `main.ts` uses for its + * other immediate destructive actions. Resolves without deleting + * anything if the user cancels. + */ + private confirmThenDeleteRemote(changeIds: ChangeId[]): Promise { + const paths = changeIds + .map(id => this.plugin.changeRepository.getById(id)?.path) + .filter((path): path is string => path !== undefined); + if (paths.length === 0) return Promise.resolve(); + const message = t('sourceControl.row.confirmDeleteRemote', { path: paths.join(', ') }); + return new Promise((resolve) => { + new ConfirmModal( + this.app, + message, + () => { void this.plugin.sourceControlActions.deleteRemote(changeIds).then(resolve); }, + () => resolve(), + ).open(); + }); + } + /** * Resolves the +/- diff stat for a change row. `local-only` reads the * already-in-memory local content from `sync.status` (no I/O, no provider diff --git a/src/ui/source-control/SourceControlView.ts b/src/ui/source-control/SourceControlView.ts index 7fb6ff5..8ef48a1 100644 --- a/src/ui/source-control/SourceControlView.ts +++ b/src/ui/source-control/SourceControlView.ts @@ -8,7 +8,7 @@ import type { ChangeId } from '../../logic/source-control/types'; import { ICONS } from '../components/icons'; import { renderDiffViewer, currentDiffLayout, rememberDiffLayout, type DiffViewerHandle } from '../components/DiffViewer'; import { renderChangeTree, renderChangeList, type ChangeTreeCallbacks } from './ChangeTree'; -import { renderChangeItem } from './ChangeItem'; +import { renderChangeItem, type RowActionKind } from './ChangeItem'; import { DiffStatProvider, type DiffStatLoadResult } from './DiffStatProvider'; import { renderFilterMenu } from './FilterMenu'; import { renderSourceControlHeader, type SourceControlWorkspaceInfo } from './SourceControlHeader'; @@ -34,6 +34,24 @@ export interface SourceControlViewCallbacks { * Queue button. */ onPull?: (changeIds: ChangeId[]) => void | Promise; + /** + * Pushes a single change immediately, bypassing the Sync Queue — the + * Repository Changes row menu's "Push local" on a kind that doesn't + * default there (e.g. `remote-modified`). + */ + onPush?: (changeIds: ChangeId[]) => void | Promise; + /** + * Deletes a single change from the remote only, immediately — the row + * menu's "Delete remote". Callers show their own confirm before invoking + * this; it does not confirm on its own. + */ + onDeleteRemote?: (changeIds: ChangeId[]) => void | Promise; + /** + * Deletes a single change from the local vault only, immediately — the + * row menu's "Delete local". Goes through Obsidian's own trash + * (`app.fileManager.trashFile`), so no separate confirm is shown here. + */ + onDeleteLocal?: (changeIds: ChangeId[]) => void | Promise; /** Triggers a view-wide refresh; the host wires this to the ViewModel's refresh delegate. */ onRefresh: () => void; /** Notified when a change is selected for diff viewing, in addition to this view's own diff pane rendering. */ @@ -289,6 +307,8 @@ export class SourceControlView { onOpenDiff: (item) => this.openDiff(item), onDownload: (item) => this.download(item), onChangeSyncAction: (item, action) => this.changeSyncAction(item, action), + onRowAction: (item, action) => this.runRowAction(item, action), + onOpenRemote: (item) => { if (this.callbacks.onOpenRemoteFile) void this.callbacks.onOpenRemoteFile(item); }, getDiffStat: (id) => this.diffStat.get(id), }; @@ -553,6 +573,20 @@ export class SourceControlView { if (this.callbacks.onPull) void this.callbacks.onPull([item.id]); } + /** + * Runs a Repository Changes row's "⋯" menu action immediately (not + * queued) — see {@link ChangeItemCallbacks.onRowAction}. Just dispatches + * to the matching single-change callback; confirmation (delete-remote) + * and the actual `SourceControlActionService` calls live at the host + * (`SourceControlItemView`), not in this pure-projection view. + */ + private runRowAction(item: SourceControlItem, action: RowActionKind): void { + if (action === 'push' && this.callbacks.onPush) void this.callbacks.onPush([item.id]); + else if (action === 'pull' && this.callbacks.onPull) void this.callbacks.onPull([item.id]); + else if (action === 'delete-remote' && this.callbacks.onDeleteRemote) void this.callbacks.onDeleteRemote([item.id]); + else if (action === 'delete-local' && this.callbacks.onDeleteLocal) void this.callbacks.onDeleteLocal([item.id]); + } + /** * Records (or clears) a Sync Queue row's explicit action override, chosen * from its {@link ChangeItemCallbacks.onChangeSyncAction} menu. Picking diff --git a/styles.css b/styles.css index 028dd73..81ad326 100644 --- a/styles.css +++ b/styles.css @@ -708,6 +708,30 @@ body.is-mobile .scv-change-download { padding: 4px; min-width: 28px; min-height: body.is-mobile .scv-change-action-label { display: none; } body.is-mobile .scv-change-action { padding: 4px; min-width: 28px; min-height: 28px; justify-content: center; } +/* ── Repository Changes row "⋯" menu ───────────────────────────────── */ +.scv-change-menu { + display: inline-flex; + align-items: center; + justify-content: center; + margin-left: auto; + padding: 2px; + background: transparent; + border: none; + border-radius: var(--radius-s); + color: var(--text-muted); + cursor: pointer; + flex-shrink: 0; +} + +.scv-change-menu:hover { + background: var(--background-modifier-hover); + color: var(--text-normal); +} + +.scv-change-menu .svg-icon { width: 14px; height: 14px; } + +body.is-mobile .scv-change-menu { min-width: 28px; min-height: 28px; } + .scv-change-rename-from { color: var(--text-faint); text-decoration: line-through; diff --git a/tests/setup.ts b/tests/setup.ts index e846f4b..63c8eb3 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -210,6 +210,10 @@ export const Modal = class { } open() { + // Real Modal appends modalEl to document.body on open(), inside a + // '.modal-container' wrapper; mirrored here so tests can find/click into + // it via document queries like any other rendered control. + document.body.appendChild(this.modalEl); const withOnOpen = this as unknown as { onOpen?: () => void }; if (typeof withOnOpen.onOpen === 'function') { withOnOpen.onOpen(); @@ -217,6 +221,7 @@ export const Modal = class { } close() { + this.modalEl.remove(); const withOnClose = this as unknown as { onClose?: () => void }; if (typeof withOnClose.onClose === 'function') { withOnClose.onClose(); diff --git a/tests/ui/source-control/SourceControlItemView.test.ts b/tests/ui/source-control/SourceControlItemView.test.ts index a6921f6..e0941e1 100644 --- a/tests/ui/source-control/SourceControlItemView.test.ts +++ b/tests/ui/source-control/SourceControlItemView.test.ts @@ -1,4 +1,4 @@ -import { beforeAll, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; import { TFile, WorkspaceLeaf } from 'obsidian'; import { SourceControlItemView, SOURCE_CONTROL_VIEW_TYPE } from '../../../src/ui/source-control/SourceControlItemView'; import { ChangeRepository } from '../../../src/logic/source-control/ChangeRepository'; @@ -24,6 +24,7 @@ function buildPlugin(kind: SyncChangeKind = 'local-only') { const push = vi.fn().mockResolvedValue(undefined); const pull = vi.fn().mockResolvedValue(undefined); const deleteRemote = vi.fn().mockResolvedValue(undefined); + const deleteLocal = vi.fn().mockResolvedValue(undefined); const loadDiffContent = vi.fn().mockResolvedValue({ remote: 'remote text', local: 'local text' }); const openDiffTab = vi.fn().mockResolvedValue(undefined); const getRemoteFileUrl = vi.fn().mockReturnValue('https://github.com/owner/repo/blob/main/a.md'); @@ -35,7 +36,7 @@ function buildPlugin(kind: SyncChangeKind = 'local-only') { pushSelectionStore: selection, operationState: operations, sourceControlViewModel: viewModel, - sourceControlActions: { sync, push, pull, deleteRemote, loadDiffContent }, + sourceControlActions: { sync, push, pull, deleteRemote, deleteLocal, loadDiffContent }, sync: { status }, syncWorkspace: { getInfo: () => ({ serviceName: 'GitHub', branch: 'main', vaultFolder: '' }), getRemoteFileUrl }, settings: { syncMetadata: {} }, @@ -44,7 +45,7 @@ function buildPlugin(kind: SyncChangeKind = 'local-only') { diffTabPath, } as unknown as GitLabFilesPush; - return { plugin, repository, selection, sync, push, pull, deleteRemote, loadDiffContent, openDiffTab, getRemoteFileUrl, status, diffTabPath }; + return { plugin, repository, selection, sync, push, pull, deleteRemote, deleteLocal, loadDiffContent, openDiffTab, getRemoteFileUrl, status, diffTabPath }; } function buildLeaf() { @@ -362,4 +363,47 @@ describe('SourceControlItemView', () => { expect(openDiffTab).toHaveBeenCalledWith('a.md', null); }); + + describe('row menu delete-remote confirmation', () => { + afterEach(() => { + document.querySelectorAll('.menu').forEach(el => el.remove()); + }); + + it('confirms before calling sourceControlActions.deleteRemote, and does not call it if cancelled', async () => { + const { plugin, deleteRemote } = buildPlugin('remote-only'); + const view = new SourceControlItemView({} as WorkspaceLeaf, plugin); + await view.onOpen(); + + const container = view.containerEl.children[1] as HTMLElement; + (container.querySelector('.scv-change-menu') as HTMLButtonElement).click(); + (Array.from(document.querySelectorAll('.menu .menu-item')) + .find(el => el.getAttribute('data-title') === 'Delete remote…') as HTMLElement).click(); + + // Confirm modal is now open; deleteRemote must not run until confirmed. + expect(deleteRemote).not.toHaveBeenCalled(); + const buttons = Array.from(document.querySelectorAll('.ssv-confirm-buttons button')); + expect(buttons.length).toBe(2); + + // Cancel: still not called. + buttons[0]?.click(); + expect(deleteRemote).not.toHaveBeenCalled(); + }); + + it('calls sourceControlActions.deleteRemote with the row id once confirmed', async () => { + const { plugin, deleteRemote } = buildPlugin('remote-only'); + const view = new SourceControlItemView({} as WorkspaceLeaf, plugin); + await view.onOpen(); + + const container = view.containerEl.children[1] as HTMLElement; + (container.querySelector('.scv-change-menu') as HTMLButtonElement).click(); + (Array.from(document.querySelectorAll('.menu .menu-item')) + .find(el => el.getAttribute('data-title') === 'Delete remote…') as HTMLElement).click(); + + const buttons = Array.from(document.querySelectorAll('.ssv-confirm-buttons button')); + buttons[1]?.click(); + await Promise.resolve(); + + expect(deleteRemote).toHaveBeenCalledWith([toChangeId('a.md')]); + }); + }); }); diff --git a/tests/ui/source-control/SourceControlView.test.ts b/tests/ui/source-control/SourceControlView.test.ts index 69dfeae..1d1db32 100644 --- a/tests/ui/source-control/SourceControlView.test.ts +++ b/tests/ui/source-control/SourceControlView.test.ts @@ -667,6 +667,112 @@ describe('SourceControlView', () => { }); }); + describe('repository row menu', () => { + afterEach(() => { document.querySelectorAll('.menu').forEach(el => el.remove()); }); + + it('renders no row menu button for a conflict or synced row', () => { + const { view } = buildView([ + { id: toChangeId('c-1'), path: 'a.md', kind: 'conflict' }, + ]); + view.render(container); + + expect(container.querySelector('.scv-changes-tree .scv-change-menu')).toBeNull(); + }); + + it('offers Push local, Use remote…, View diff, Add to Sync Queue, and Delete local… for a local-modified row', () => { + const { view } = buildView([ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }, + ]); + view.render(container); + + (container.querySelector('.scv-changes-tree .scv-change-menu') as HTMLButtonElement).click(); + + const titles = Array.from(document.querySelectorAll('.menu .menu-item')).map(el => el.getAttribute('data-title')); + expect(titles).toEqual(['Push local', 'Use remote…', 'View diff', 'Add to Sync Queue', 'Delete local…']); + }); + + it('offers Download, Delete remote…, Add to Sync Queue, and Open remote for a remote-only row', () => { + const { view } = buildView([ + { id: toChangeId('c-1'), path: 'a.md', kind: 'remote-only' }, + ]); + view.render(container); + + (container.querySelector('.scv-changes-tree .scv-change-menu') as HTMLButtonElement).click(); + + const titles = Array.from(document.querySelectorAll('.menu .menu-item')).map(el => el.getAttribute('data-title')); + expect(titles).toEqual(['Download', 'Delete remote…', 'Add to Sync Queue', 'Open remote']); + }); + + it('routes "Push local" to onPush with just that row\'s id', () => { + const onPush = vi.fn(); + const { view } = buildView( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }], + { onPush }, + ); + view.render(container); + + (container.querySelector('.scv-changes-tree .scv-change-menu') as HTMLButtonElement).click(); + (Array.from(document.querySelectorAll('.menu .menu-item')) + .find(el => el.getAttribute('data-title') === 'Push local') as HTMLElement).click(); + + expect(onPush).toHaveBeenCalledWith([toChangeId('c-1')]); + }); + + it('routes "Delete remote…" to onDeleteRemote', () => { + const onDeleteRemote = vi.fn(); + const { view } = buildView( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'remote-only' }], + { onDeleteRemote }, + ); + view.render(container); + + (container.querySelector('.scv-changes-tree .scv-change-menu') as HTMLButtonElement).click(); + (Array.from(document.querySelectorAll('.menu .menu-item')) + .find(el => el.getAttribute('data-title') === 'Delete remote…') as HTMLElement).click(); + + expect(onDeleteRemote).toHaveBeenCalledWith([toChangeId('c-1')]); + }); + + it('routes "Add to Sync Queue" to selection instead of any immediate action', () => { + const { view, selection } = buildView([ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }, + ]); + view.render(container); + + (container.querySelector('.scv-changes-tree .scv-change-menu') as HTMLButtonElement).click(); + (Array.from(document.querySelectorAll('.menu .menu-item')) + .find(el => el.getAttribute('data-title') === 'Add to Sync Queue') as HTMLElement).click(); + + expect(selection.isIncluded(toChangeId('c-1'))).toBe(true); + }); + + it('routes "Open remote" to onOpenRemoteFile', () => { + const onOpenRemoteFile = vi.fn(); + const { view } = buildView( + [{ id: toChangeId('c-1'), path: 'a.md', kind: 'remote-only' }], + { onOpenRemoteFile }, + ); + view.render(container); + + (container.querySelector('.scv-changes-tree .scv-change-menu') as HTMLButtonElement).click(); + (Array.from(document.querySelectorAll('.menu .menu-item')) + .find(el => el.getAttribute('data-title') === 'Open remote') as HTMLElement).click(); + + expect(onOpenRemoteFile).toHaveBeenCalledWith(expect.objectContaining({ id: toChangeId('c-1') })); + }); + + it('does not render the row menu on a Sync Queue row (the compact action control supersedes it)', () => { + const { view, selection } = buildView([ + { id: toChangeId('c-1'), path: 'a.md', kind: 'local-modified' }, + ]); + selection.selectForSync(toChangeId('c-1')); + view.render(container); + + const queueSection = container.querySelector('.scv-selected-section') as HTMLElement; + expect(queueSection.querySelector('.scv-change-menu')).toBeNull(); + }); + }); + describe('operation status', () => { it('renders the running indicator for a change with an in-flight operation', () => { const { view, operations } = buildView([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }]); From c4a198ce7d2ba881eea4a400ceb2a007782c3b8f Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 02:57:18 +0000 Subject: [PATCH 12/26] fix(sync-plan): mark each plan row with its section's direction icon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merged Sync review can list additions/modifications/moves, downloads, and deletions in one long scroll; a row far from its section heading previously carried no cue of its own direction. Each file row now repeats the section's existing icon (already shown once in the heading) inline before the path — presentation only, no new grouping, no selector: classification, conflict resolution, and bucket planning are unchanged. --- src/ui/SyncPlanModal.ts | 8 +++++++- styles.css | 17 +++++++++++++++++ tests/ui/SyncPlanModal.test.ts | 20 ++++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/ui/SyncPlanModal.ts b/src/ui/SyncPlanModal.ts index f222219..e95f54e 100644 --- a/src/ui/SyncPlanModal.ts +++ b/src/ui/SyncPlanModal.ts @@ -96,7 +96,13 @@ export class SyncPlanModal extends Modal { const list = section.createEl('ul', { cls: 'sync-plan-file-list' }); for (const entry of entries) { const item = list.createEl('li', { cls: 'sync-plan-file-item' }); - item.createSpan({ cls: 'sync-plan-file-path', text: entry.path }); + // Mirrors the section heading's icon on each row — with a long + // mixed-direction plan (the merged Sync review), a section label + // scrolled out of view shouldn't leave a row's direction + // ambiguous. + const row = item.createDiv({ cls: 'sync-plan-file-row' }); + setIcon(row.createSpan({ cls: 'sync-plan-file-icon' }), icon); + row.createSpan({ cls: 'sync-plan-file-path', text: entry.path }); if (entry.movedFrom) { item.createSpan({ cls: 'sync-plan-file-moved-from', text: t('syncPlanModal.movedFrom', { path: entry.movedFrom }) }); } diff --git a/styles.css b/styles.css index 81ad326..6c44620 100644 --- a/styles.css +++ b/styles.css @@ -1629,9 +1629,26 @@ body.is-mobile .gfs-conflict-modal--batch .batch-conflict-row-actions { flex-direction: column; } +.sync-plan-file-row { + display: flex; + align-items: center; + gap: 6px; +} + +.sync-plan-file-icon { + display: flex; + flex-shrink: 0; + color: var(--text-faint); +} + +.sync-plan-file-icon .svg-icon { width: 11px; height: 11px; } + +.sync-plan-section.is-destructive .sync-plan-file-icon { color: var(--text-error); } + .sync-plan-file-moved-from { color: var(--text-muted); font-size: 0.9em; + padding-left: 17px; } .sync-plan-buttons { diff --git a/tests/ui/SyncPlanModal.test.ts b/tests/ui/SyncPlanModal.test.ts index ede7b7a..d1ad62c 100644 --- a/tests/ui/SyncPlanModal.test.ts +++ b/tests/ui/SyncPlanModal.test.ts @@ -28,6 +28,26 @@ describe('SyncPlanModal', () => { expect(modal.contentEl.querySelector('.sync-plan-section.is-destructive')).toBeNull(); }); + it('prefixes every file row with a direction icon, so a row reads unambiguously even with its section heading scrolled out of view', () => { + const plan: SyncPlan = { + ...emptyPlan(), + modifications: [{ path: 'push-me.md', name: 'push-me.md' }], + downloads: [{ path: 'pull-me.md', name: 'pull-me.md' }], + deletions: [{ path: 'gone.md', name: 'gone.md' }], + }; + const modal = new SyncPlanModal(new App(), plan, 'sync', vi.fn()); + modal.contentEl = createContainer(); + + modal.onOpen(); + + const rows = Array.from(modal.contentEl.querySelectorAll('.sync-plan-file-row')); + expect(rows.length).toBe(3); + for (const row of rows) { + expect(row.querySelector('.sync-plan-file-icon')).not.toBeNull(); + expect(row.querySelector('.sync-plan-file-path')).not.toBeNull(); + } + }); + it('shows the destructive deletion warning when the plan includes deletions', () => { const plan: SyncPlan = { ...emptyPlan(), deletions: [{ path: 'gone.md', name: 'gone.md' }] }; const modal = new SyncPlanModal(new App(), plan, 'delete', vi.fn()); From d91fa6e1faa20d81ff3f75bc2fefa9888de9b7b3 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 02:58:37 +0000 Subject: [PATCH 13/26] docs: record explicit sync intent session in progress.md --- progress.md | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/progress.md b/progress.md index f430950..59226c3 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-08-31 -**Active Feature:** Issue #143 — reduce redundant real-provider E2E round trips. Working tree changes complete, uncommitted. -**Branch / PR:** Current working branch; no commit or push created in this session. +**Last Updated:** 2026-09-01 +**Active Feature:** restore explicit per-file sync actions (no tracked issue number). All 7 planned commits landed and each individually passes lint/tests/build. +**Branch / PR:** `claude/fix-source-control-explicit-sync-intent`, based on `claude/fix-mobile-diff-rendering-and-responsive-layout` (itself 1 commit ahead of `main`). Not yet pushed or opened as a PR. -**Scope:** E2E fixtures, verifier helpers, and tests only; production `SyncManager` and provider batching behavior remain unchanged. +**Scope:** `SyncSelectionStore`/`ChangeActionPolicy` (per-change action overrides + resolution), `SourceControlViewModel` (resolved `syncAction`/`hasActionOverride` projection), Sync Queue grouping/row controls, `SourceControlActionService.sync()` (now takes `SyncIntentRequest[]`), a new Repository Changes row "⋯" menu, and `SyncPlanModal` per-row direction icons. Deliberately did not touch `DiffViewer.ts`, mobile diff lifecycle, or E2E cleanup — that's the base branch's prior work. -Below that: the previous "Outstanding Items"/"Verification Evidence" entries track separate, still-open work on PR #129 / `claude/source-control-foundation` — not superseded by this entry. +**Next:** push the branch and open the PR (title `fix(source-control): restore explicit per-file sync actions`); no further planned work outstanding. + +Below that: the previous "Outstanding Items"/"Verification Evidence" entries track separate, still-open work on PR #129 / `claude/source-control-foundation` and Issue #143 — not superseded by this entry, carried over from the base branch history. ## Outstanding Items @@ -19,6 +21,12 @@ Below that: the previous "Outstanding Items"/"Verification Evidence" entries tra ## Verification Evidence +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. +- Final state: `npx eslint .` — 0 errors. `npx vitest run` — 68 files / 914 tests passed. `npm run build` — passed. +- Not run this session: the real-provider E2E suite (`vitest.e2e.config.ts`) — only typechecked (two call sites updated for the new `SyncIntentRequest[]` shape), not executed; needs provisioned credentials. + This session (Issue #143 — reduce redundant real-provider E2E round trips): - `SyncManagerFixture.makeSettings()` and the standalone SyncManager E2E settings now use the selected provider identity, instead of hard-coding Gitea. From f5540311e5eacb7a08a9d27630308f2f19d4ddad Mon Sep 17 00:00:00 2001 From: ClaudiaFang Date: Tue, 1 Sep 2026 11:34:52 +0800 Subject: [PATCH 14/26] refactor(source-control): isolate sync intent orchestration Separate queued sync intent execution from immediate Source Control actions, reconcile stale action overrides on repository snapshot changes, and keep ViewModel reads observational. --- docs/source-control.md | 73 +++-- src/logic/source-control/ChangeRepository.ts | 21 +- .../SourceControlActionService.ts | 304 ++++-------------- .../source-control/SourceControlViewModel.ts | 96 ++---- src/logic/source-control/SyncIntent.ts | 13 + .../source-control/SyncIntentExecutor.ts | 290 +++++++++++++++++ .../source-control/SyncSelectionStore.ts | 55 ++-- .../SyncSelectionStoreReconcile.test.ts | 42 +++ 8 files changed, 524 insertions(+), 370 deletions(-) create mode 100644 src/logic/source-control/SyncIntent.ts create mode 100644 src/logic/source-control/SyncIntentExecutor.ts create mode 100644 tests/logic/source-control/SyncSelectionStoreReconcile.test.ts diff --git a/docs/source-control.md b/docs/source-control.md index 93ed5fb..47ba6b9 100644 --- a/docs/source-control.md +++ b/docs/source-control.md @@ -1,44 +1,57 @@ # Source Control — Current Architecture -The Source Control side panel is the plugin's only sync UI. There is no -separate "sync status" view; `docs/source-control-refactor/` describes the -historical migration into this architecture and is not current guidance. +The Source Control side panel is the plugin's only sync UI. Historical +migration notes live under `docs/source-control-refactor/`; they are not +current implementation guidance. ## Call chain -``` -SourceControlItemView (src/ui/source-control/SourceControlItemView.ts) - └─ SourceControlView (src/ui/source-control/SourceControlView.ts) - └─ SourceControlActionService (src/logic/source-control/SourceControlActionService.ts) - └─ SyncWorkspace (src/logic/sync/SyncWorkspace.ts) - └─ SyncManager + executors (src/logic/sync/, e.g. PushExecutor, - PullExecutor, RemoteDeleteExecutor) +```text +SourceControlItemView + └─ SourceControlView + ├─ SourceControlViewModel # read-side projection + └─ SourceControlActionService # immediate action facade + ├─ SyncIntentExecutor # Sync Queue use-case only + └─ SyncWorkspace # immediate actions + └─ SyncManager + executors + └─ GitServiceInterface ``` -- `SourceControlItemView` is the `ItemView` Obsidian mounts; it owns no - rendering logic itself and delegates to `SourceControlView`. -- `SourceControlView` renders the change tree, Sync Queue, and diff surfaces - (`src/ui/components/`, `src/ui/source-control/DiffTabView.ts`), and turns - clicks into calls on `SourceControlActionService`. -- `SourceControlActionService` converts Source Control intent (push / pull / - delete-remote / delete-local / resolve-conflict) into `SyncWorkspace` calls - and reports outcome via `OperationState`. It never talks to a git provider - directly. -- `SyncWorkspace` is the execution boundary: it drives the real `SyncManager` - and provider-mutating executors (`PushExecutor`, `PullExecutor`, - `RemoteDeleteExecutor`, etc.), which in turn call `GitServiceInterface` - (`src/services/`). +## Responsibility boundaries + +- `ChangeRepository` is the authoritative Source Control snapshot populated + from `sync.status`. Snapshot replacements notify dependent read-side state. +- `SyncSelectionStore` owns queued selection plus explicit per-change action + overrides. It reconciles stale selection/overrides when the repository + snapshot changes. +- `SourceControlViewModel` is a read-only projection. `getState()` must not + mutate selection or execution state. +- `SourceControlActionService` is the UI-facing facade for immediate push, + pull, delete, conflict resolution, diff loading, and the stable `sync()` + entry point. +- `SyncIntentExecutor` owns the Sync Queue workflow: resolve current intent, + bucket by action, build one merged plan, confirm once, commit the remote + mutation bucket once, apply the local pull bucket, and aggregate results. +- `SyncWorkspace` remains the execution boundary. Source Control code never + talks directly to a provider. + +## Sync Queue invariant + +One Sync click produces one explicit-intent workflow. Requested action +choices are revalidated against the change's current kind before execution; +a stale/illegal override falls back to the current default. Remote mutations +(push/move/delete/keep-local/keep-remote) are committed as one provider +batch, while pulls are local-only and applied after that remote bucket. ## Compatibility identifiers (do not remove) -- `SOURCE_CONTROL_VIEW_TYPE = 'sync-status-view'` — kept so pinned leaves and - saved workspace layouts from before the Source Control migration resolve to - the current `SourceControlItemView` instead of breaking. -- The `open-sync-status` command id — same reason; it already routes to - `activateSourceControlView()`. +- `SOURCE_CONTROL_VIEW_TYPE = 'sync-status-view'` — retained so saved/pinned + leaves from before the Source Control migration continue to resolve. +- `open-sync-status` command id — retained for the same compatibility reason; + it routes to the current Source Control view. ## Legacy surface (removed, do not reintroduce) `SyncStatusView` and `ui/sync-status/*` were the pre-migration UI and no -longer exist in `src/`. An ESLint `no-restricted-imports` rule -(`eslint.config.*`) blocks reintroducing imports from those paths. +longer exist in `src/`. ESLint restrictions prevent those imports from being +reintroduced. diff --git a/src/logic/source-control/ChangeRepository.ts b/src/logic/source-control/ChangeRepository.ts index 2463e33..c1505a6 100644 --- a/src/logic/source-control/ChangeRepository.ts +++ b/src/logic/source-control/ChangeRepository.ts @@ -1,16 +1,20 @@ import type { ChangeId, SyncChange } from './types'; +type ChangeRepositoryListener = (changes: readonly SyncChange[]) => void; + /** - * Read-side lookup for the current set of pending `SyncChange`s. Holds no - * sync/business logic of its own — it's populated wholesale (`replace`) by - * whatever assembles `SyncChange[]` from the sync domain, and exists purely - * to give the ViewModel and UI O(1) lookup by id or path instead of scanning - * an array. + * Read-side lookup for the current set of Source Control changes. + * + * The repository is populated wholesale from the sync.status pipeline and + * exposes one snapshot-change notification so dependent state stores can + * reconcile when that source of truth changes. It still owns no sync or + * provider behavior. */ export class ChangeRepository { private changes: SyncChange[] = []; private readonly byId = new Map(); private readonly byPath = new Map(); + private readonly listeners = new Set(); /** Replaces the full change set, e.g. after a status refresh. */ replace(changes: readonly SyncChange[]): void { @@ -21,6 +25,13 @@ export class ChangeRepository { this.byId.set(change.id, change); this.byPath.set(change.path, change); } + for (const listener of this.listeners) listener(this.changes); + } + + /** Subscribes to authoritative snapshot replacements. */ + subscribe(listener: ChangeRepositoryListener): () => void { + this.listeners.add(listener); + return () => this.listeners.delete(listener); } getAll(): SyncChange[] { diff --git a/src/logic/source-control/SourceControlActionService.ts b/src/logic/source-control/SourceControlActionService.ts index f1e8d34..ec4788c 100644 --- a/src/logic/source-control/SourceControlActionService.ts +++ b/src/logic/source-control/SourceControlActionService.ts @@ -1,28 +1,17 @@ -import type { PlannedPushBatch } from '../sync/PushCoordinator'; import type { SyncWorkspace } from '../sync/SyncWorkspace'; -import { isSyncPlanEmpty, type DeleteQueueEntry, type PushResults, type SyncPlan, type SyncPlanEntry } from '../sync/types'; import { type SyncExecutionResult, type SyncResultNotificationPort } from './SyncResultNotifier'; -import { resolveSyncAction, type SyncAction } from './ChangeActionPolicy'; import type { ChangeRepository } from './ChangeRepository'; import type { OperationState } from './OperationState'; import type { SourceControlItem } from './SourceControlViewModel'; +import { SyncIntentExecutor } from './SyncIntentExecutor'; +import type { SyncIntentRequest } from './SyncIntent'; import type { ChangeId, SyncChange } from './types'; +export type { SyncIntentRequest } from './SyncIntent'; + /** Which side wins when resolving a change in the 'conflict' state. */ export type ConflictResolution = 'local' | 'remote'; -/** - * One change to sync, with the caller's explicit action choice if it made - * one. `action` omitted (or no longer legal for the change's current kind — - * see {@link resolveSyncAction}) means "use the kind's default", so a stale - * intent from a UI snapshot can never force an action the change can't - * support. - */ -export interface SyncIntentRequest { - changeId: ChangeId; - action?: SyncAction; -} - /** Diff payload the Source Control diff pane can render directly (text-only; binary/symlink changes resolve to `null`). */ export interface SourceControlDiffContent { remote: string; @@ -30,29 +19,33 @@ export interface SourceControlDiffContent { } /** - * Converts Source Control user intent (push / pull / delete-remote / - * delete-local / resolve-conflict on one or more `ChangeId`s) into calls - * against `SyncWorkspace` — the existing `SyncManager`-backed execution - * boundary already used by the sync-status UI — per - * docs/source-control-refactor/phase-2-action-unification.md. + * Application facade for immediate Source Control actions. + * + * Immediate row actions (push / pull / delete / conflict / diff) stay here. + * The Sync Queue's multi-step intent workflow is delegated to + * {@link SyncIntentExecutor}, so this facade no longer owns plan merging, + * confirmation, remote-bucket execution, pull-bucket execution, and result + * aggregation at the same time. * - * Per that doc's rules, this service DOES convert user intent into the call - * `SyncWorkspace`/`SyncManager` need (effectively "build the SyncPlan"), but - * it never talks to a Git provider directly and never (re-)classifies - * changes — it only resolves `ChangeId` -> `SyncChange` via the Phase 1 - * `ChangeRepository` and reports per-change outcome through the Phase 1 - * `OperationState`. Unknown/stale `ChangeId`s (e.g. a change that dropped out - * between the UI snapshot and the click) are silently skipped rather than - * throwing, since the repository is the single source of truth for what's - * still actionable. + * Neither layer talks to a Git provider directly; SyncWorkspace remains the + * execution boundary. */ export class SourceControlActionService { + private readonly syncIntentExecutor: SyncIntentExecutor; + constructor( private readonly changes: ChangeRepository, private readonly operations: OperationState, private readonly workspace: SyncWorkspace, private readonly syncResultNotifier: SyncResultNotificationPort = { notify: () => {} }, - ) {} + ) { + this.syncIntentExecutor = new SyncIntentExecutor( + changes, + operations, + workspace, + syncResultNotifier, + ); + } /** Pushes one or more changes (single push and batch push share this path). */ async push(changeIds: readonly ChangeId[]): Promise { @@ -63,7 +56,7 @@ export class SourceControlActionService { 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')); + this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); } catch { this.failAll(targets); } @@ -78,7 +71,7 @@ export class SourceControlActionService { 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')); + this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); } catch { this.failAll(targets); } @@ -93,190 +86,22 @@ export class SourceControlActionService { 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')); + this.finishAll(targets, path => failed.has(path) ? 'failed' : 'success'); } catch { this.failAll(targets); } } /** - * Syncs one or more changes as a single Sync Plan — the Sync Queue - * button's only entry point. Splits the requested intents by - * {@link resolveSyncAction} (an explicit per-change action if the caller - * gave one and it's still legal, otherwise the kind's default) into - * push/delete-remote/pull buckets, plans each without mutating anything, - * merges the result into one `SyncPlan`, shows exactly one confirm, and — - * if confirmed — commits the whole remote mutation set (pushes + moves + - * deletions) through `SyncWorkspace.commitResolvedBatch` as one provider - * call, then applies the pull bucket (zero-commit, local-only) - * separately. This is the fix for the "one Sync produces two remote - * commits" bug: previously the Sync Queue routed push/pull/delete-remote - * through three independent `SyncWorkspace` calls, each committing on its - * own. + * Executes the whole Sync Queue as one explicit-intent workflow. + * Kept as the stable UI-facing facade; orchestration lives in + * SyncIntentExecutor. */ async sync(intents: readonly SyncIntentRequest[]): Promise { - const resolved = this.resolveIntents(intents); - if (resolved.length === 0) return; - const targets = resolved.map(entry => entry.change); - - const pushTargets: SyncChange[] = []; - const deleteTargets: SyncChange[] = []; - const pullTargets: SyncChange[] = []; - for (const { change, action } of resolved) { - if (action === 'pull') pullTargets.push(change); - else if (action === 'delete-remote') deleteTargets.push(change); - else pushTargets.push(change); - } - - let plan: { planned: PlannedPushBatch; confirmed: boolean } | null; - try { - plan = await this.planSync(pushTargets, pullTargets, deleteTargets); - } catch { - this.failAll(targets); - this.syncResultNotifier.notify({ ...SourceControlActionService.emptyExecutionResult(), failed: targets.length }); - return; - } - if (!plan || !plan.confirmed) return; - const { planned } = plan; - - this.startAll(targets); - const summary = SourceControlActionService.emptyExecutionResult(); - if (planned.pushes.length > 0 || planned.moves.length > 0 || planned.keepRemote.length > 0 || planned.keepLocal.length > 0 || deleteTargets.length > 0) { - await this.commitRemoteBucket(planned, pushTargets, deleteTargets, summary); - } - if (pullTargets.length > 0) { - await this.applyPullBucket(pullTargets, summary); - } - this.syncResultNotifier.notify(summary); - } - - /** Builds and confirms the merged Sync Plan; returns null if there's nothing to do or the user cancelled. */ - private async planSync( - pushTargets: readonly SyncChange[], - pullTargets: readonly SyncChange[], - deleteTargets: readonly SyncChange[], - ): Promise<{ planned: PlannedPushBatch; confirmed: boolean } | null> { - const planned = pushTargets.length > 0 - ? await this.workspace.planPush(pushTargets.map(target => target.path)) - : SourceControlActionService.emptyPlannedBatch(); - // A cancelled batch-conflict resolution is a separate interactive - // step that happens before the merged plan is even shown; honor it - // the same way pushFiles() does, without touching anything. - if (planned.cancelled) return null; - - const pullPlan = pullTargets.length > 0 - ? await this.workspace.planPull(pullTargets.map(target => target.path)) - : SourceControlActionService.emptyPlan(); - - const deletions: SyncPlanEntry[] = deleteTargets.map(target => ({ path: target.path, name: basename(target.path) })); - const mergedPlan: SyncPlan = { - additions: planned.reviewPlan.additions, - modifications: planned.reviewPlan.modifications, - moves: planned.reviewPlan.moves, - deletions, - downloads: [...pullPlan.additions, ...pullPlan.modifications], - acceptedRemote: planned.reviewPlan.acceptedRemote, - skippedConflicts: planned.reviewPlan.skippedConflicts, - }; - if (isSyncPlanEmpty(mergedPlan)) return null; - - const confirmed = await this.workspace.confirmPlan(mergedPlan, 'sync'); - return { planned, confirmed }; - } - - /** Commits the merged push/move/delete-remote bucket; a failure here only fails that bucket, not any already-applied pull. */ - private async commitRemoteBucket( - planned: PlannedPushBatch, - pushTargets: readonly SyncChange[], - deleteTargets: readonly SyncChange[], - summary: SyncExecutionResult, - ): Promise { - try { - const deleteEntries: DeleteQueueEntry[] = deleteTargets.map(target => ({ - path: target.path, - name: basename(target.path), - repoPath: this.workspace.toRepoPath(target.path), - })); - const results: PushResults = { - success: planned.immediate.success, - added: 0, - updated: planned.immediate.updated, - failed: planned.immediate.failed, - conflicts: 0, - resolvedConflicts: 0, - skippedConflicts: 0, - errors: [...planned.immediate.errors], - syncedPaths: [...planned.immediate.syncedPaths], - }; - await this.workspace.commitResolvedBatch(planned.pushes, planned.moves, deleteEntries, planned.keepRemote, planned.keepLocal, results); - const failed = new Set(results.errors.map(error => error.file)); - this.finishAll([...pushTargets, ...deleteTargets], path => (failed.has(path) ? 'failed' : 'success')); - this.addRemoteResult(summary, planned, deleteEntries, results); - } catch { - this.failAll([...pushTargets, ...deleteTargets]); - summary.failed += pushTargets.length + deleteTargets.length; - } - } - - /** Applies the zero-commit pull bucket; a failure here only fails the pull targets, not any already-committed remote bucket. */ - private async applyPullBucket(pullTargets: readonly SyncChange[], summary: SyncExecutionResult): Promise { - try { - const pullResults = await this.workspace.applyPull(pullTargets.map(target => target.path), { notify: false }); - const failed = new Set(pullResults.errors.map(error => error.file)); - this.finishAll(pullTargets, path => (failed.has(path) ? 'failed' : 'success')); - summary.downloaded += pullResults.added + pullResults.updated; - summary.failed += pullResults.failed; - summary.conflicts += pullResults.conflicts; - summary.errors.push(...pullResults.errors); - } catch { - this.failAll(pullTargets); - summary.failed += pullTargets.length; - } - } - - private static emptyPlannedBatch(): PlannedPushBatch { - return { - reviewPlan: { additions: [], modifications: [], deletions: [], moves: [] }, - pushes: [], - moves: [], - keepRemote: [], - keepLocal: [], - skippedConflicts: 0, - conflictedPaths: [], - cancelled: false, - immediate: { success: 0, updated: 0, failed: 0, errors: [], syncedPaths: [] }, - }; - } - - private static emptyPlan(): SyncPlan { - return { additions: [], modifications: [], deletions: [], moves: [] }; + await this.syncIntentExecutor.execute(intents); } - private static emptyExecutionResult(): SyncExecutionResult { - return { added: 0, updated: 0, moved: 0, deleted: 0, downloaded: 0, acceptedRemote: 0, failed: 0, conflicts: 0, skippedConflicts: 0, errors: [] }; - } - - private addRemoteResult( - summary: SyncExecutionResult, - planned: PlannedPushBatch, - deletions: readonly DeleteQueueEntry[], - results: PushResults, - ): void { - const failedPaths = new Set(results.errors.map(error => error.file)); - summary.added += planned.pushes.filter(entry => !entry.existingSha && !failedPaths.has(entry.path)).length; - summary.updated += planned.pushes.filter(entry => entry.existingSha && !failedPaths.has(entry.path)).length + planned.immediate.updated; - summary.moved += planned.moves.filter(entry => !failedPaths.has(entry.path)).length; - summary.deleted += deletions.filter(entry => !failedPaths.has(entry.path)).length; - summary.acceptedRemote += planned.keepRemote - .filter(conflict => !failedPaths.has(conflict.path)) - .length; - summary.failed += results.failed; - summary.conflicts += results.conflicts; - summary.skippedConflicts += results.skippedConflicts; - summary.errors.push(...results.errors); - } - - /** Deletes one or more changes from the local vault only. No batch primitive exists on `SyncWorkspace`, so each runs independently and one failure doesn't block the rest. */ + /** 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) { @@ -291,11 +116,9 @@ export class SourceControlActionService { } /** - * Resolves a single change in the 'conflict' state by pushing the local - * copy (local wins) or pulling the reviewed remote copy (remote wins). - * Remote resolution goes through the explicit acceptRemoteConflict - * boundary, which applies the reviewed remote blob without re-running the - * planner — so no second conflict modal can appear. + * Resolves a single conflict by keeping the local or reviewed remote + * version. Remote resolution uses the explicit acceptRemoteConflict + * boundary so it does not re-enter planning and show a second modal. */ async resolveConflict(changeId: ChangeId, resolution: ConflictResolution): Promise { const change = this.changes.getById(changeId); @@ -305,33 +128,30 @@ export class SourceControlActionService { try { if (resolution === 'local') { const results = await this.workspace.push([change.path]); - if (results.errors.length > 0) throw new Error(results.errors.map(error => error.error).join('; ')); + if (results.errors.length > 0) { + throw new Error(results.errors.map(error => error.error).join('; ')); + } this.operations.succeed(changeId); - this.syncResultNotifier.notify({ ...SourceControlActionService.emptyExecutionResult(), updated: 1 }); + this.syncResultNotifier.notify({ ...emptyExecutionResult(), updated: 1 }); } else { await this.workspace.acceptRemoteConflict(change.path); this.operations.succeed(changeId); - this.syncResultNotifier.notify({ ...SourceControlActionService.emptyExecutionResult(), acceptedRemote: 1 }); + this.syncResultNotifier.notify({ ...emptyExecutionResult(), acceptedRemote: 1 }); } } catch { this.operations.fail(changeId); - this.syncResultNotifier.notify({ ...SourceControlActionService.emptyExecutionResult(), failed: 1 }); + this.syncResultNotifier.notify({ ...emptyExecutionResult(), failed: 1 }); } } - /** - * Supplies `SourceControlView`'s `loadDiffContent` callback: delegates to - * the existing `SyncWorkspace.getDiff`/`SyncDiffService` (no new diff - * logic) and resolves to `null` for binary/symlink changes, which the - * text-only diff pane can't render. - */ + /** Loads text diff content for the Source Control diff surface. */ async loadDiffContent(item: SourceControlItem): Promise { const diff = await this.workspace.getDiff(item.path); if (typeof diff.remoteContent !== 'string' || typeof diff.localContent !== 'string') return null; return { remote: diff.remoteContent, local: diff.localContent }; } - /** Resolves ChangeIds to their current SyncChange, dropping any that are no longer known to the repository. */ + /** Resolves ChangeIds against the repository's current snapshot, dropping stale ids. */ private resolve(changeIds: readonly ChangeId[]): SyncChange[] { const targets: SyncChange[] = []; for (const id of changeIds) { @@ -341,29 +161,14 @@ export class SourceControlActionService { return targets; } - /** - * Resolves sync intents to their current `SyncChange` plus the action - * each actually syncs as, dropping any change no longer known to the - * repository. Legality is re-checked here against the change's *current* - * kind (not whatever it was when the caller snapshotted it), via - * {@link resolveSyncAction} — so a stale intent degrades to the default - * instead of ever forcing an action the current kind can't support. - */ - private resolveIntents(intents: readonly SyncIntentRequest[]): Array<{ change: SyncChange; action: SyncAction }> { - const resolved: Array<{ change: SyncChange; action: SyncAction }> = []; - for (const intent of intents) { - const change = this.changes.getById(intent.changeId); - if (!change) continue; - resolved.push({ change, action: resolveSyncAction(change.kind, intent.action) }); - } - return resolved; - } - private startAll(targets: readonly SyncChange[]): void { for (const target of targets) this.operations.start(target.id); } - private finishAll(targets: readonly SyncChange[], statusFor: (path: string) => 'success' | 'failed'): void { + private finishAll( + targets: readonly SyncChange[], + statusFor: (path: string) => 'success' | 'failed', + ): void { for (const target of targets) { if (statusFor(target.path) === 'success') this.operations.succeed(target.id); else this.operations.fail(target.id); @@ -375,8 +180,17 @@ export class SourceControlActionService { } } -/** Last path segment of a change path, for the Sync Plan's deletions section. */ -function basename(path: string): string { - const slash = path.lastIndexOf('/'); - return slash === -1 ? path : path.slice(slash + 1); +function emptyExecutionResult(): SyncExecutionResult { + return { + added: 0, + updated: 0, + moved: 0, + deleted: 0, + downloaded: 0, + acceptedRemote: 0, + failed: 0, + conflicts: 0, + skippedConflicts: 0, + errors: [], + }; } diff --git a/src/logic/source-control/SourceControlViewModel.ts b/src/logic/source-control/SourceControlViewModel.ts index a3964bc..849581d 100644 --- a/src/logic/source-control/SourceControlViewModel.ts +++ b/src/logic/source-control/SourceControlViewModel.ts @@ -8,7 +8,7 @@ import type { SyncSelectionStore } from './SyncSelectionStore'; import { matchesFilter, type SourceControlFilter } from './SourceControlFilter'; import type { ChangeId, SyncChange, SyncChangeKind } from './types'; -/** One row of UI-ready state for a change: its own facts plus derived selection/operation status. */ +/** One UI-ready row: repository facts plus derived selection/operation state. */ export interface SourceControlItem { id: ChangeId; path: string; @@ -16,57 +16,29 @@ export interface SourceControlItem { kind: SyncChangeKind; isSelectedForSync: boolean; operationStatus: OperationStatus; - /** The action this change actually syncs as — the user's override if still legal for `kind`, otherwise the default. */ + /** Current effective queue action: a still-legal override or the kind default. */ syncAction: SyncAction; - /** Whether `syncAction` came from a still-legal user override, as opposed to the kind's default. */ + /** True only when the effective action came from a still-legal user override. */ hasActionOverride: boolean; } -/** The complete state the Source Control UI needs to render for a given filter. */ +/** The complete state the Source Control UI needs to render for a filter. */ export interface SourceControlViewState { filter: SourceControlFilter; items: SourceControlItem[]; - /** - * The actionable changes the user has currently selected for push, as - * full row items — the working sync queue. Empty when nothing is - * selected. Reuses the same `selected + non-synced` definition as - * `buildSummary.readyToPush` so the "SYNC QUEUE (N)" section and the Sync - * button count can't drift. - */ syncQueue: SourceControlItem[]; - /** Current view-wide refresh status, surfaced so the header can render its states. */ refreshStatus: RefreshStatus; - /** Single-source counts from {@link buildSummary} — the view never recomputes these. */ counts: SourceControlCounts; } /** - * Combines `SyncChange[]` (via `ChangeRepository`), `SyncSelectionStore`, and - * `OperationState` into a single UI-ready snapshot. Holds no sync behavior of - * its own — it's a pure projection, so `SyncManager`/`SyncPlanner`/`SyncExecutor` - * stay untouched and the UI never needs to reach past this layer. - * - * Every count the UI shows comes from one place: {@link buildSummary}. The - * ViewModel only projects items for the active filter and forwards the - * summary's counts unchanged, so the filter menu, section headers, and tree - * can never drift apart. + * Read-only projection of repository, selection, operation, and refresh state + * into UI-ready snapshots. * - * `showSynced` governs whether the synced bucket is surfaced: when false the - * synced count is reported as `0` and the `synced` filter yields no items, - * matching the "Show synced" toggle (default off). - * - * `toItem` has one side effect for the same reason `refresh` does: a stale - * action override (recorded when a change was e.g. `local-modified`, now - * stranded because the change became `local-only`) is cleared on the - * selection store as soon as a projection notices it's no longer legal, - * rather than left to resurface if the kind later reverts. - * - * The other non-projection responsibility is {@link refresh}: it delegates to an - * injected refresh callback (wired to `SyncWorkspace.refresh()` in `main.ts`) - * and drives the injected {@link RefreshState} holder so the UI can surface - * loading/failed states. It holds no provider or refresh logic of its own, - * keeping the event-driven pipeline (`sync.status` → `ChangeRepository` → - * ViewModel → UI) intact — refresh never becomes a second population path. + * The constructor wires selection-intent reconciliation to authoritative + * ChangeRepository replacements. Cleanup therefore happens on the write-side + * repository lifecycle, while repeated getState() calls remain observational + * and never mutate queue intent. */ export class SourceControlViewModel { constructor( @@ -75,14 +47,13 @@ export class SourceControlViewModel { private readonly operations: OperationState, private readonly refreshSource: () => Promise, private readonly refreshState: RefreshState, - ) {} + ) { + this.changes.subscribe(changes => this.selectionStore.reconcile(changes)); + } /** - * The sync-selection store, exposed so the view can toggle/clear - * selection without holding its own reference and reaching past the - * ViewModel. Reached via `viewModel.selection` - * (`selectForSync`/`deselectFromSync`/`selectMany`/`deselectMany`/ - * `getSelectedChangeIds`). + * Existing UI mutation boundary for queue selection. Kept for this PR to + * avoid mixing a renderer API redesign into the intent/execution cleanup. */ get selection(): SyncSelectionStore { return this.selectionStore; } @@ -94,22 +65,20 @@ export class SourceControlViewModel { .filter(() => this.isRenderable(filter, showSynced)) .map(change => this.toItem(change)); const syncQueue = summary.readyToPush.map(change => this.toItem(change)); - return { filter, items, syncQueue, refreshStatus: this.refreshState.get(), counts: summary.counts }; + + return { + filter, + items, + syncQueue, + refreshStatus: this.refreshState.get(), + counts: summary.counts, + }; } /** - * Triggers a view-wide refresh by delegating to the injected refresh - * source (the Sync Status service boundary) and tracking its lifecycle on - * the {@link RefreshState} holder so the header can render "Refreshing…" - * / a failed state. Refresh republishes `sync.status`, so the existing - * subscription repopulates `ChangeRepository` — this never becomes a - * second population path. - * - * The {@link RefreshReason} is recorded on the {@link RefreshState} - * holder purely for observability ("Last checked" + why); it does not - * change what the refresh does. Defaults to `'manual'` (the Refresh - * button); callers pass `'startup'`/`'local-change'`/`'sync-complete'` to - * surface a non-manual trigger. + * Triggers a view-wide refresh through the injected source and records + * only its presentation lifecycle. Repository population still happens + * exclusively through the existing sync.status publish subscription. */ async refresh(reason: RefreshReason = 'manual'): Promise { this.refreshState.start(reason); @@ -123,9 +92,6 @@ export class SourceControlViewModel { } private isRenderable(filter: SourceControlFilter, showSynced: boolean): boolean { - // Synced rows only render under the `synced` filter, and only when the - // user has opted in via "Show synced". `all`/`changes`/etc. already - // exclude synced via matchesFilter, so this only gates the synced view. return !(filter === 'synced' && !showSynced); } @@ -133,13 +99,7 @@ export class SourceControlViewModel { const storedOverride = this.selectionStore.getActionOverride(change.id); const syncAction = resolveSyncAction(change.kind, storedOverride); const hasActionOverride = storedOverride !== undefined && storedOverride === syncAction; - // The change's kind moved on since the override was recorded (e.g. a - // stored 'pull' on what's now local-only) — resolveSyncAction already - // fell back to the default, so drop the now-meaningless override - // rather than let it linger and resurface once the kind reverts. - if (storedOverride !== undefined && !hasActionOverride) { - this.selectionStore.clearActionOverride(change.id); - } + return { id: change.id, path: change.path, @@ -151,4 +111,4 @@ export class SourceControlViewModel { hasActionOverride, }; } -} \ No newline at end of file +} diff --git a/src/logic/source-control/SyncIntent.ts b/src/logic/source-control/SyncIntent.ts new file mode 100644 index 0000000..c83409d --- /dev/null +++ b/src/logic/source-control/SyncIntent.ts @@ -0,0 +1,13 @@ +import type { SyncAction } from './ChangeActionPolicy'; +import type { ChangeId } from './types'; + +/** + * One queued Source Control intent. `action` is present only when the user + * explicitly chose a legal non-default action for the change at the time the + * queue snapshot was built. Execution always re-validates it against the + * repository's current change kind before doing any work. + */ +export interface SyncIntentRequest { + changeId: ChangeId; + action?: SyncAction; +} diff --git a/src/logic/source-control/SyncIntentExecutor.ts b/src/logic/source-control/SyncIntentExecutor.ts new file mode 100644 index 0000000..9db08ec --- /dev/null +++ b/src/logic/source-control/SyncIntentExecutor.ts @@ -0,0 +1,290 @@ +import type { PlannedPushBatch } from '../sync/PushCoordinator'; +import type { SyncWorkspace } from '../sync/SyncWorkspace'; +import { + isSyncPlanEmpty, + type DeleteQueueEntry, + type PushResults, + type SyncPlan, + type SyncPlanEntry, +} from '../sync/types'; +import { resolveSyncAction, type SyncAction } from './ChangeActionPolicy'; +import type { ChangeRepository } from './ChangeRepository'; +import type { OperationState } from './OperationState'; +import type { SyncExecutionResult, SyncResultNotificationPort } from './SyncResultNotifier'; +import type { SyncIntentRequest } from './SyncIntent'; +import type { SyncChange } from './types'; + +interface ResolvedSyncIntent { + change: SyncChange; + action: SyncAction; +} + +interface SyncIntentBuckets { + push: SyncChange[]; + pull: SyncChange[]; + deleteRemote: SyncChange[]; +} + +interface ConfirmedSyncPlan { + plannedPush: PlannedPushBatch; + confirmed: boolean; +} + +/** + * Executes the Sync Queue use-case from explicit user intent. + * + * This class owns only the queued/batched workflow: resolve each ChangeId + * against the current repository snapshot, re-validate the requested action, + * build one merged review plan, confirm once, commit the remote mutation + * bucket once, then apply the local-only pull bucket. Immediate row actions + * remain on SourceControlActionService. + * + * Keeping this orchestration behind a dedicated boundary prevents + * SourceControlActionService from becoming the place where every Source + * Control concern accumulates, while preserving the existing SyncWorkspace + * execution boundary and provider behavior. + */ +export class SyncIntentExecutor { + constructor( + private readonly changes: ChangeRepository, + private readonly operations: OperationState, + private readonly workspace: SyncWorkspace, + private readonly notifier: SyncResultNotificationPort = { notify: () => {} }, + ) {} + + async execute(intents: readonly SyncIntentRequest[]): Promise { + const resolved = this.resolveIntents(intents); + if (resolved.length === 0) return; + + const targets = resolved.map(entry => entry.change); + const buckets = this.bucket(resolved); + + let plan: ConfirmedSyncPlan | null; + try { + plan = await this.planAndConfirm(buckets); + } catch { + this.failAll(targets); + this.notifier.notify({ ...emptyExecutionResult(), failed: targets.length }); + return; + } + + if (!plan || !plan.confirmed) return; + + this.startAll(targets); + const summary = emptyExecutionResult(); + + if (hasRemoteMutations(plan.plannedPush, buckets.deleteRemote)) { + await this.commitRemoteBucket(plan.plannedPush, buckets.push, buckets.deleteRemote, summary); + } + if (buckets.pull.length > 0) { + await this.applyPullBucket(buckets.pull, summary); + } + + this.notifier.notify(summary); + } + + private resolveIntents(intents: readonly SyncIntentRequest[]): ResolvedSyncIntent[] { + const resolved: ResolvedSyncIntent[] = []; + for (const intent of intents) { + const change = this.changes.getById(intent.changeId); + if (!change) continue; + resolved.push({ + change, + action: resolveSyncAction(change.kind, intent.action), + }); + } + return resolved; + } + + private bucket(intents: readonly ResolvedSyncIntent[]): SyncIntentBuckets { + const buckets: SyncIntentBuckets = { push: [], pull: [], deleteRemote: [] }; + for (const { change, action } of intents) { + if (action === 'pull') buckets.pull.push(change); + else if (action === 'delete-remote') buckets.deleteRemote.push(change); + else buckets.push.push(change); + } + return buckets; + } + + private async planAndConfirm(buckets: SyncIntentBuckets): Promise { + const plannedPush = buckets.push.length > 0 + ? await this.workspace.planPush(buckets.push.map(change => change.path)) + : emptyPlannedBatch(); + + // Batch conflict resolution is an interactive planning step. If the + // user cancels it, no merged review modal or mutation should follow. + if (plannedPush.cancelled) return null; + + const pullPlan = buckets.pull.length > 0 + ? await this.workspace.planPull(buckets.pull.map(change => change.path)) + : emptyPlan(); + + const deletions: SyncPlanEntry[] = buckets.deleteRemote.map(change => ({ + path: change.path, + name: basename(change.path), + })); + const mergedPlan: SyncPlan = { + additions: plannedPush.reviewPlan.additions, + modifications: plannedPush.reviewPlan.modifications, + moves: plannedPush.reviewPlan.moves, + deletions, + downloads: [...pullPlan.additions, ...pullPlan.modifications], + acceptedRemote: plannedPush.reviewPlan.acceptedRemote, + skippedConflicts: plannedPush.reviewPlan.skippedConflicts, + }; + + if (isSyncPlanEmpty(mergedPlan)) return null; + + return { + plannedPush, + confirmed: await this.workspace.confirmPlan(mergedPlan, 'sync'), + }; + } + + private async commitRemoteBucket( + plannedPush: PlannedPushBatch, + pushTargets: readonly SyncChange[], + deleteTargets: readonly SyncChange[], + summary: SyncExecutionResult, + ): Promise { + const targets = [...pushTargets, ...deleteTargets]; + try { + const deleteEntries: DeleteQueueEntry[] = deleteTargets.map(change => ({ + path: change.path, + name: basename(change.path), + repoPath: this.workspace.toRepoPath(change.path), + })); + const results: PushResults = { + success: plannedPush.immediate.success, + added: 0, + updated: plannedPush.immediate.updated, + failed: plannedPush.immediate.failed, + conflicts: 0, + resolvedConflicts: 0, + skippedConflicts: 0, + errors: [...plannedPush.immediate.errors], + syncedPaths: [...plannedPush.immediate.syncedPaths], + }; + + await this.workspace.commitResolvedBatch( + plannedPush.pushes, + plannedPush.moves, + deleteEntries, + plannedPush.keepRemote, + plannedPush.keepLocal, + results, + ); + + 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 { + this.failAll(targets); + summary.failed += targets.length; + } + } + + private async applyPullBucket( + pullTargets: readonly SyncChange[], + summary: SyncExecutionResult, + ): Promise { + try { + const results = await this.workspace.applyPull( + pullTargets.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'); + summary.downloaded += results.added + results.updated; + summary.failed += results.failed; + summary.conflicts += results.conflicts; + summary.errors.push(...results.errors); + } catch { + this.failAll(pullTargets); + summary.failed += pullTargets.length; + } + } + + private startAll(targets: readonly SyncChange[]): void { + for (const target of targets) this.operations.start(target.id); + } + + private finishAll( + targets: readonly SyncChange[], + statusFor: (path: string) => 'success' | 'failed', + ): void { + for (const target of targets) { + if (statusFor(target.path) === 'success') this.operations.succeed(target.id); + else this.operations.fail(target.id); + } + } + + private failAll(targets: readonly SyncChange[]): void { + for (const target of targets) this.operations.fail(target.id); + } +} + +function hasRemoteMutations(planned: PlannedPushBatch, deletions: readonly SyncChange[]): boolean { + return planned.pushes.length > 0 + || planned.moves.length > 0 + || planned.keepRemote.length > 0 + || planned.keepLocal.length > 0 + || deletions.length > 0; +} + +function emptyPlannedBatch(): PlannedPushBatch { + return { + reviewPlan: { additions: [], modifications: [], deletions: [], moves: [] }, + pushes: [], + moves: [], + keepRemote: [], + keepLocal: [], + skippedConflicts: 0, + conflictedPaths: [], + cancelled: false, + immediate: { success: 0, updated: 0, failed: 0, errors: [], syncedPaths: [] }, + }; +} + +function emptyPlan(): SyncPlan { + return { additions: [], modifications: [], deletions: [], moves: [] }; +} + +function emptyExecutionResult(): SyncExecutionResult { + return { + added: 0, + updated: 0, + moved: 0, + deleted: 0, + downloaded: 0, + acceptedRemote: 0, + failed: 0, + conflicts: 0, + skippedConflicts: 0, + errors: [], + }; +} + +function addRemoteResult( + summary: SyncExecutionResult, + planned: PlannedPushBatch, + deletions: readonly DeleteQueueEntry[], + results: PushResults, +): void { + const failedPaths = new Set(results.errors.map(error => error.file)); + summary.added += planned.pushes.filter(entry => !entry.existingSha && !failedPaths.has(entry.path)).length; + summary.updated += planned.pushes.filter(entry => entry.existingSha && !failedPaths.has(entry.path)).length + + planned.immediate.updated; + summary.moved += planned.moves.filter(entry => !failedPaths.has(entry.path)).length; + summary.deleted += deletions.filter(entry => !failedPaths.has(entry.path)).length; + summary.acceptedRemote += planned.keepRemote.filter(conflict => !failedPaths.has(conflict.path)).length; + summary.failed += results.failed; + summary.conflicts += results.conflicts; + summary.skippedConflicts += results.skippedConflicts; + summary.errors.push(...results.errors); +} + +function basename(path: string): string { + const slash = path.lastIndexOf('/'); + return slash === -1 ? path : path.slice(slash + 1); +} diff --git a/src/logic/source-control/SyncSelectionStore.ts b/src/logic/source-control/SyncSelectionStore.ts index 9a7c3bd..c380a9b 100644 --- a/src/logic/source-control/SyncSelectionStore.ts +++ b/src/logic/source-control/SyncSelectionStore.ts @@ -1,23 +1,14 @@ -import type { ChangeId } from './types'; -import type { SyncAction } from './ChangeActionPolicy'; +import { resolveSyncAction, type SyncAction } from './ChangeActionPolicy'; +import type { ChangeId, SyncChange } from './types'; /** - * Tracks which pending sync changes are selected for the Sync Queue — - * independent of the underlying change/plan model and of any UI. Named for - * "selected for sync" rather than "push" since the Sync Queue it backs holds - * push, pull, and delete-remote candidates alike (a queued `remote-only` row - * pulls, a queued `local-deleted` row deletes remotely by default). Also - * deliberately avoids VCS stage/unstage terminology since this isn't a - * staging area. + * Tracks which pending changes are selected for the Sync Queue and the + * user's optional per-change action override. * - * Keyed by ChangeId rather than path so a rename/move doesn't drop the - * selection. - * - * Also holds an optional per-change action override — the user explicitly - * picking pull instead of the default push, say — keyed the same way. - * Legality of an override (is 'pull' even valid for this change's kind) is - * not this store's concern; that's `ChangeActionPolicy`'s job, applied at - * read time via `resolveSyncAction`. + * Keyed by ChangeId rather than path so a rename/move does not drop intent. + * The store owns lifecycle cleanup for that intent: when a refreshed change + * disappears, or an override is no longer legal for its current kind, the + * stale state is discarded here instead of during ViewModel projection. */ export class SyncSelectionStore { private readonly selected = new Set(); @@ -32,12 +23,10 @@ export class SyncSelectionStore { this.actionOverrides.delete(changeId); } - /** Selects a batch of changes for sync in one call (folder "select all"). */ selectMany(changeIds: readonly ChangeId[]): void { for (const id of changeIds) this.selected.add(id); } - /** Deselects a batch of changes from sync in one call ("clear queue" / folder deselect). */ deselectMany(changeIds: readonly ChangeId[]): void { for (const id of changeIds) { this.selected.delete(id); @@ -53,12 +42,10 @@ export class SyncSelectionStore { return [...this.selected]; } - /** Records the user's explicit action choice for a change (e.g. pull instead of the default push). */ setActionOverride(changeId: ChangeId, action: SyncAction): void { this.actionOverrides.set(changeId, action); } - /** Reverts a change back to its default action. */ clearActionOverride(changeId: ChangeId): void { this.actionOverrides.delete(changeId); } @@ -67,7 +54,25 @@ export class SyncSelectionStore { return this.actionOverrides.get(changeId); } - /** Drops selections for change ids that are no longer present, keeping the rest. */ + /** + * Reconciles queued intent with a freshly published repository snapshot. + * Missing ids are removed and action overrides are revalidated against + * each change's current kind. This is the write-side lifecycle boundary; + * read-only ViewModel projection must not clean state as a side effect. + */ + reconcile(changes: readonly SyncChange[]): void { + const currentById = new Map(changes.map(change => [change.id, change] as const)); + this.refresh([...currentById.keys()]); + + for (const [changeId, override] of this.actionOverrides) { + const change = currentById.get(changeId); + if (change && resolveSyncAction(change.kind, override) !== override) { + this.actionOverrides.delete(changeId); + } + } + } + + /** Drops selections for ids that are no longer present, keeping the rest. */ refresh(currentChangeIds: readonly ChangeId[]): void { const present = new Set(currentChangeIds); for (const changeId of this.selected) { @@ -76,5 +81,11 @@ export class SyncSelectionStore { this.actionOverrides.delete(changeId); } } + + // Defensive cleanup for callers/tests that recorded an override + // without first selecting the row. + for (const changeId of this.actionOverrides.keys()) { + if (!present.has(changeId)) this.actionOverrides.delete(changeId); + } } } diff --git a/tests/logic/source-control/SyncSelectionStoreReconcile.test.ts b/tests/logic/source-control/SyncSelectionStoreReconcile.test.ts new file mode 100644 index 0000000..6027946 --- /dev/null +++ b/tests/logic/source-control/SyncSelectionStoreReconcile.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from 'vitest'; +import { SyncSelectionStore } from '../../../src/logic/source-control/SyncSelectionStore'; +import { toChangeId, type SyncChange } from '../../../src/logic/source-control/types'; + +describe('SyncSelectionStore.reconcile', () => { + it('clears an action override that is no longer legal for the current change kind', () => { + const store = new SyncSelectionStore(); + const id = toChangeId('change-a'); + store.selectForSync(id); + store.setActionOverride(id, 'pull'); + + const current: SyncChange[] = [{ id, path: 'a.md', kind: 'local-only' }]; + store.reconcile(current); + + expect(store.isIncluded(id)).toBe(true); + expect(store.getActionOverride(id)).toBeUndefined(); + }); + + it('keeps a still-legal explicit override', () => { + const store = new SyncSelectionStore(); + const id = toChangeId('change-a'); + store.selectForSync(id); + store.setActionOverride(id, 'pull'); + + const current: SyncChange[] = [{ id, path: 'a.md', kind: 'local-modified' }]; + store.reconcile(current); + + expect(store.getActionOverride(id)).toBe('pull'); + }); + + it('drops both selection and override when the change disappeared', () => { + const store = new SyncSelectionStore(); + const id = toChangeId('change-a'); + store.selectForSync(id); + store.setActionOverride(id, 'pull'); + + store.reconcile([]); + + expect(store.isIncluded(id)).toBe(false); + expect(store.getActionOverride(id)).toBeUndefined(); + }); +}); From 1c69aed959954c427c910ccd3d9eb54ecc74de45 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 03:46:19 +0000 Subject: [PATCH 15/26] fix(source-control): remove Conflict filter chip and dedupe sync time in header The header showed both "Last sync" and "Last checked" times, which read as duplicated; keep only "Last checked" and drop the now-unused Conflict filter chip (conflicts remain reachable via the default Needs Sync / All views). Co-Authored-By: Claude Sonnet 5 --- package-lock.json | 4 +-- src/i18n/locales/en.ts | 3 --- src/i18n/locales/zh-cn.ts | 3 --- src/i18n/locales/zh-tw.ts | 3 --- src/ui/source-control/FilterMenu.ts | 9 +++---- src/ui/source-control/SourceControlHeader.ts | 10 ------- .../source-control/SourceControlItemView.ts | 4 +-- tests/ui/source-control/FilterMenu.test.ts | 8 +++--- .../source-control/SourceControlView.test.ts | 26 ++++++++----------- 9 files changed, 22 insertions(+), 48 deletions(-) diff --git a/package-lock.json b/package-lock.json index 92e0efe..9fa9e04 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "git-file-sync", - "version": "1.5.9", + "version": "1.6.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "git-file-sync", - "version": "1.5.9", + "version": "1.6.0", "license": "MIT", "dependencies": { "ignore": "^7.0.6", diff --git a/src/i18n/locales/en.ts b/src/i18n/locales/en.ts index 99b2bb5..bac2a95 100644 --- a/src/i18n/locales/en.ts +++ b/src/i18n/locales/en.ts @@ -178,7 +178,6 @@ const en = { 'sourceControl.filter.changes': 'Changes', 'sourceControl.filter.local': 'Local', 'sourceControl.filter.remote': 'Incoming', - 'sourceControl.filter.conflict': 'Conflict', 'sourceControl.filter.readyToPush': 'Ready to Push', 'sourceControl.filter.remoteChanges': 'Incoming', 'sourceControl.filter.conflicts': 'Conflicts', @@ -234,10 +233,8 @@ const en = { 'sourceControl.detail.back': 'Back', 'sourceControl.mobile.filesSelected': '{count} files selected', 'sourceControl.mobile.sync': 'Sync', - 'sourceControl.info.lastSync': 'Last sync: {time}', 'sourceControl.info.lastChecked': 'Last checked: {time}', 'sourceControl.info.justChecked': 'Last checked: just now', - 'sourceControl.info.neverSynced': 'Never synced', 'sourceControl.search.placeholder': 'Filter by path…', 'sourceControl.search.clear': 'Clear filter', 'sourceControl.folder.selectAll': 'Select all in folder', diff --git a/src/i18n/locales/zh-cn.ts b/src/i18n/locales/zh-cn.ts index 7c6e810..60898b6 100644 --- a/src/i18n/locales/zh-cn.ts +++ b/src/i18n/locales/zh-cn.ts @@ -180,7 +180,6 @@ const zhCn: Partial> = { 'sourceControl.filter.changes': '更改', 'sourceControl.filter.local': '本地', 'sourceControl.filter.remote': '传入', - 'sourceControl.filter.conflict': '冲突', 'sourceControl.filter.readyToPush': '待推送', 'sourceControl.filter.remoteChanges': '传入', 'sourceControl.filter.conflicts': '冲突', @@ -236,10 +235,8 @@ const zhCn: Partial> = { 'sourceControl.detail.back': '返回', 'sourceControl.mobile.filesSelected': '已选 {count} 个文件', 'sourceControl.mobile.sync': '同步', - 'sourceControl.info.lastSync': '上次同步：{time}', 'sourceControl.info.lastChecked': '上次检查：{time}', 'sourceControl.info.justChecked': '上次检查：刚刚', - 'sourceControl.info.neverSynced': '尚未同步', 'sourceControl.search.placeholder': '按路径过滤…', 'sourceControl.search.clear': '清除过滤', 'sourceControl.folder.selectAll': '选取文件夹内全部项目', diff --git a/src/i18n/locales/zh-tw.ts b/src/i18n/locales/zh-tw.ts index d43b7f6..dec9fc0 100644 --- a/src/i18n/locales/zh-tw.ts +++ b/src/i18n/locales/zh-tw.ts @@ -180,7 +180,6 @@ const zhTw: Partial> = { 'sourceControl.filter.changes': '變更', 'sourceControl.filter.local': '本地', 'sourceControl.filter.remote': '傳入', - 'sourceControl.filter.conflict': '衝突', 'sourceControl.filter.readyToPush': '待推送', 'sourceControl.filter.remoteChanges': '傳入', 'sourceControl.filter.conflicts': '衝突', @@ -236,10 +235,8 @@ const zhTw: Partial> = { 'sourceControl.detail.back': '返回', 'sourceControl.mobile.filesSelected': '已選 {count} 個檔案', 'sourceControl.mobile.sync': '同步', - 'sourceControl.info.lastSync': '上次同步：{time}', 'sourceControl.info.lastChecked': '上次檢查：{time}', 'sourceControl.info.justChecked': '上次檢查：剛剛', - 'sourceControl.info.neverSynced': '尚未同步', 'sourceControl.search.placeholder': '以路徑過濾…', 'sourceControl.search.clear': '清除過濾', 'sourceControl.folder.selectAll': '選取資料夾內全部項目', diff --git a/src/ui/source-control/FilterMenu.ts b/src/ui/source-control/FilterMenu.ts index 6208dec..1abd5c7 100644 --- a/src/ui/source-control/FilterMenu.ts +++ b/src/ui/source-control/FilterMenu.ts @@ -15,7 +15,7 @@ import type { SourceControlCounts } from '../../logic/source-control/SourceContr * bucket in the view (see `SourceControlView`) rather than via a domain * change. Surfaced as an opt-in overview; the default stays on Needs Sync * so a quiet workspace stays quiet. - * - **Incoming / Conflict / Synced** — the matching domain filters (chip id stays `remote`; the label reads "Incoming" — a file only on the remote, or changed only on the remote, is something coming *in*). + * - **Incoming / Synced** — the matching domain filters (chip id stays `remote`; the label reads "Incoming" — a file only on the remote, or changed only on the remote, is something coming *in*). * * "Local" (domain `changes`) is intentionally not a chip: Needs Sync already * covers local-side changes, and a standalone local-only view added a @@ -30,12 +30,11 @@ export interface FilterChip { count: (counts: SourceControlCounts) => number; } -/** The five filter chips, in display order. */ +/** The four filter chips, in display order. */ export const FILTER_CHIPS: readonly FilterChip[] = [ { id: 'all', filter: 'all', showSynced: true, labelKey: 'sourceControl.filter.all', count: c => c.all + c.synced }, { id: 'needsSync', filter: 'all', showSynced: false, labelKey: 'sourceControl.filter.needsSync', count: c => c.all }, { id: 'remote', filter: 'remote-changes', showSynced: false, labelKey: 'sourceControl.filter.remote', count: c => c['remote-changes'] }, - { id: 'conflict', filter: 'conflicts', showSynced: false, labelKey: 'sourceControl.filter.conflict', count: c => c.conflicts }, { id: 'synced', filter: 'synced', showSynced: true, labelKey: 'sourceControl.filter.synced', count: c => c.synced }, ]; @@ -55,8 +54,8 @@ export interface FilterMenuOptions { } /** - * Renders the Source Control filter row: five chips — All / Needs Sync / - * Incoming / Conflict / Synced. On mobile a single `` dropdown * replaces the chips (same chip ids, counts inline as "Label (N)"). * * Per-filter counts come straight from the ViewModel's single-source counts diff --git a/src/ui/source-control/SourceControlHeader.ts b/src/ui/source-control/SourceControlHeader.ts index 7d7431a..9ef6cdd 100644 --- a/src/ui/source-control/SourceControlHeader.ts +++ b/src/ui/source-control/SourceControlHeader.ts @@ -8,8 +8,6 @@ export interface SourceControlWorkspaceInfo { serviceName: string; branch: string; vaultFolder: string; - /** Epoch ms of the most recent successful push/pull, or 0 if nothing has synced yet. */ - lastSyncTime: number; /** * Epoch ms the Source Control view last completed a status refresh * (any reason — manual, startup, local-change), or 0 if it hasn't @@ -104,14 +102,6 @@ function renderInfoStrip(container: HTMLElement, info: SourceControlWorkspaceInf folder.createSpan({ text: ` ${info.vaultFolder}` }); } - strip.createSpan({ cls: 'scv-info-sep', text: '·' }); - strip.createSpan({ - cls: 'scv-info-time', - text: info.lastSyncTime > 0 - ? t('sourceControl.info.lastSync', { time: new Date(info.lastSyncTime).toLocaleTimeString() }) - : t('sourceControl.info.neverSynced'), - }); - if (info.lastCheckedAt > 0) { strip.createSpan({ cls: 'scv-info-sep', text: '·' }); const elapsed = Date.now() - info.lastCheckedAt; diff --git a/src/ui/source-control/SourceControlItemView.ts b/src/ui/source-control/SourceControlItemView.ts index 5297542..4e9bf24 100644 --- a/src/ui/source-control/SourceControlItemView.ts +++ b/src/ui/source-control/SourceControlItemView.ts @@ -219,10 +219,8 @@ export class SourceControlItemView extends ItemView { private getWorkspaceInfo(): SourceControlWorkspaceInfo { const info = this.plugin.syncWorkspace.getInfo(); - const lastSyncTime = Object.values(this.plugin.settings.syncMetadata) - .reduce((latest, metadata) => Math.max(latest, metadata.lastSyncedAt), 0); const lastCheckedAt = this.plugin.refreshState.getLastCheckedAt(); - return { ...info, lastSyncTime, lastCheckedAt }; + return { ...info, lastCheckedAt }; } getViewType(): string { return SOURCE_CONTROL_VIEW_TYPE; } diff --git a/tests/ui/source-control/FilterMenu.test.ts b/tests/ui/source-control/FilterMenu.test.ts index 02339e4..c604cc8 100644 --- a/tests/ui/source-control/FilterMenu.test.ts +++ b/tests/ui/source-control/FilterMenu.test.ts @@ -19,18 +19,18 @@ describe('renderFilterMenu', () => { callbacks = { onFilterChange: vi.fn() }; }); - it('renders the five chips (All/Needs Sync/Incoming/Conflict/Synced)', () => { + it('renders the four chips (All/Needs Sync/Incoming/Synced)', () => { renderFilterMenu(container, { filter: 'all', showSynced: false }, zeroCounts, callbacks); const chips = Array.from(container.querySelectorAll('.scv-filter-option')).map(el => el.getAttribute('data-filter')); - expect(chips).toEqual(['all', 'needsSync', 'remote', 'conflict', 'synced']); + expect(chips).toEqual(['all', 'needsSync', 'remote', 'synced']); }); it('labels the chips with their display names', () => { renderFilterMenu(container, { filter: 'all', showSynced: false }, zeroCounts, callbacks); const labels = Array.from(container.querySelectorAll('.scv-filter-option .scv-filter-label')).map(el => el.textContent); - expect(labels).toEqual(['All', 'Needs Sync', 'Incoming', 'Conflict', 'Synced']); + expect(labels).toEqual(['All', 'Needs Sync', 'Incoming', 'Synced']); }); it('marks the current (filter, showSynced) chip as active', () => { @@ -66,6 +66,6 @@ describe('renderFilterMenu', () => { expect(container.querySelector('.scv-filter-dropdown')).not.toBeNull(); expect(container.querySelector('.scv-filter-option')).toBeNull(); const options = Array.from(container.querySelectorAll('.scv-filter-dropdown option')).map(o => (o as HTMLOptionElement).value); - expect(options).toEqual(['all', 'needsSync', 'remote', 'conflict', 'synced']); + expect(options).toEqual(['all', 'needsSync', 'remote', 'synced']); }); }); \ No newline at end of file diff --git a/tests/ui/source-control/SourceControlView.test.ts b/tests/ui/source-control/SourceControlView.test.ts index 1d1db32..dd082b6 100644 --- a/tests/ui/source-control/SourceControlView.test.ts +++ b/tests/ui/source-control/SourceControlView.test.ts @@ -25,7 +25,6 @@ function buildView(changes: SyncChange[], callbacks: Partial { it('shows a flat tree (no sections) once a specific filter is selected', () => { const { view } = buildView([ - { id: toChangeId('c-1'), path: 'a.md', kind: 'conflict' }, + { id: toChangeId('c-1'), path: 'a.md', kind: 'remote-only' }, { id: toChangeId('c-2'), path: 'b.md', kind: 'local-only' }, ]); view.render(container); - (container.querySelector('.scv-filter-option[data-filter="conflict"]') as HTMLButtonElement).click(); + (container.querySelector('.scv-filter-option[data-filter="remote"]') as HTMLButtonElement).click(); expect(container.querySelectorAll('.scv-section')).toHaveLength(0); expect(container.querySelectorAll('.scv-change-item')).toHaveLength(1); - expect(view.getFilter()).toBe('conflicts'); + expect(view.getFilter()).toBe('remote-changes'); }); it('shows the empty state when the active filter has no items', () => { const { view } = buildView([{ id: toChangeId('c-1'), path: 'a.md', kind: 'local-only' }]); view.render(container); - (container.querySelector('.scv-filter-option[data-filter="conflict"]') as HTMLButtonElement).click(); + (container.querySelector('.scv-filter-option[data-filter="remote"]') as HTMLButtonElement).click(); expect(container.querySelector('.scv-empty')).not.toBeNull(); }); @@ -1584,7 +1582,7 @@ describe('SourceControlView', () => { const view = new SourceControlView( viewModel, { onSync: vi.fn(), onRefresh: vi.fn() }, - () => ({ serviceName: 'GitHub', branch: 'main', vaultFolder: '', lastSyncTime: 0, lastCheckedAt }), + () => ({ serviceName: 'GitHub', branch: 'main', vaultFolder: '', lastCheckedAt }), ); return { view, refreshState }; } @@ -1594,9 +1592,7 @@ describe('SourceControlView', () => { view.render(container); const infoTimes = container.querySelectorAll('.scv-info-time'); - // Only the "Never synced" line is present; no "Last checked" line. - expect(infoTimes).toHaveLength(1); - expect(infoTimes[0]?.textContent).toBe('Never synced'); + expect(infoTimes).toHaveLength(0); }); it('shows "Last checked: just now" when the last refresh was within a minute', () => { @@ -1604,8 +1600,8 @@ describe('SourceControlView', () => { view.render(container); const infoTimes = container.querySelectorAll('.scv-info-time'); - expect(infoTimes).toHaveLength(2); - expect(infoTimes[1]?.textContent).toBe('Last checked: just now'); + expect(infoTimes).toHaveLength(1); + expect(infoTimes[0]?.textContent).toBe('Last checked: just now'); }); it('shows "Last checked: