From fd6d7ec4798830e27119594701dc493b9ecbf438 Mon Sep 17 00:00:00 2001 From: tianyao Date: Tue, 1 Sep 2026 00:40:43 +0000 Subject: [PATCH 1/5] 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 1d8e5f4d..1f5587f7 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 fffc04b9..5a6de09f 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 2d782ee6..05ca5814 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 6cb2ec0e..dd850fe9 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 c63b50bf..cc490923 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 221e1c0a..b650df2f 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 2/5] 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 91607d5a..d20bf5da 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 3/5] 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 57f16cce..718c0688 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 4/5] 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 ca14c94d..cc23b501 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 f9623bdd..11b09ad9 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 f9299175..1f89e9ec 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 a1da7103..56b4db12 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 a928ff3e..d5cc3fd8 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 00000000..93ed5fb4 --- /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 5/5] 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 ce85aea6..4d3975fe 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'"); + }); });