refactor(source-control): isolate explicit sync intent orchestration - #150
Closed
ClaudiaFang wants to merge 15 commits into
Closed
ClaudiaFang wants to merge 15 commits into
ClaudiaFang wants to merge 15 commits into
Conversation
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.
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.
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).
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.
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.
…ection 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.
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.
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.
…ueue 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.
…n phone 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.
…Changes rows 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.
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.
Separate queued sync intent execution from immediate Source Control actions, reconcile stale action overrides on repository snapshot changes, and keep ViewModel reads observational.
… 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 <noreply@anthropic.com>
ClaudiaFang
deleted the
claude/fix-source-control-explicit-sync-intent
branch
September 1, 2026 04:02
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The Source Control path had started accumulating unrelated responsibilities in the same classes: immediate row actions, queued sync intent resolution, merged-plan construction, execution/result aggregation, and read-side cleanup during ViewModel projection.
This PR keeps the current Source Control behavior but makes the intent/execution boundaries explicit.
Architecture cleanup
SyncIntentRequestinto a dedicated intent typeSyncIntentExecutorSourceControlActionServiceas the stable UI-facing facade for immediate actions andsync()SourceControlViewModel.getState()observational instead of clearing stale overrides while renderingdocs/source-control.mdBehavior preserved
SourceControlActionService.sync()API remains compatibleTests
SourceControlActionServicetests continue to cover the facade and the full queued-sync workflowSyncSelectionStore.reconcilecoverage for illegal overrides, legal overrides, and removed changesIntentionally not in this PR
SourceControlView.ts/ChangeItem.tsUI refactorSyncWorkspace,SyncManager, coordinators, or provider servicesRelease intent for the architecture commit is
refactor, notfeat.