Skip to content

feat(sync): sync pipeline implementation (#27)#53

Merged
jaybarden1 merged 7 commits into
mainfrom
feature/27-sync-pipeline
May 29, 2026
Merged

feat(sync): sync pipeline implementation (#27)#53
jaybarden1 merged 7 commits into
mainfrom
feature/27-sync-pipeline

Conversation

@jaybarden1
Copy link
Copy Markdown
Contributor

Summary

  • Implements the full six-step sync pipeline: RemoteFolderEnumeratorRemoteDeletionDetectorLocalDeletionDetectorDownloadJobBuilderLocalChangeDetectorJobExecutor (parallel bounded-channel execution via SyncPipeline)
  • Introduces SyncRule domain record, SyncConflict with typed value-object IDs, SyncError discriminated union, and all supporting interfaces
  • Resolves all 15 error-severity and 8 warning-severity findings from two rounds of code review

Key fixes applied during review

  • FileClassifier — token matching now uses exact Equals (not substring Contains); false-positive test added
  • SyncSchedulerSyncAccountAsync result handled via MatchAsync; SyncCompleted fires on success only; failures logged
  • App.axaml.csservices.AddHttpClient() added; IHttpClientFactory now registered for GraphService and HttpDownloader
  • GraphServiceSystem.IO.* replaced with IFileSystem; raw new HttpClient() replaced with IHttpClientFactory; GetClientForToken no longer throws inside Match; UploadChunksAsync has full retry loop (429 / 404 / backoff)
  • SyncWorker — calls GetDownloadUrlAsync before download; accountId/driveId threaded through from caller
  • HttpDownloaderFileMode.Create truncates on re-download; 429 double-sleep eliminated
  • SyncPipeline — global Interlocked.Increment counter replaces per-worker counter
  • Pipeline step interfacesIRemoteFolderEnumerator, IRemoteDeletionDetector, ILocalDeletionDetector, IDownloadJobBuilder, ILocalChangeDetector, IJobExecutor added; SyncService injects all by interface

Test plan

  • dotnet build — 0 errors, 0 warnings
  • dotnet run --project test/AStar.Dev.CloudSyncFunctional.Tests.Unit — 230 passed, 0 failed
  • Manual smoke test: sign in, select folders, trigger sync, verify files downloaded to local path
  • Verify SyncCompleted event is not raised when sync fails (scheduler fix)
  • Verify /photographs/img.jpg is not classified under a "photo" keyword rule (classifier fix)

🤖 Generated with Claude Code

jaybarden1 and others added 3 commits May 28, 2026 14:24
Tests reference SyncError, SyncRuleEvaluator, FileClassifier, SyncJob, SyncConflict,
ISyncService, ISyncScheduler — none of which exist yet. All tests fail to compile.

Compile error count: 5 new errors (CS0234 — namespace AStar.Dev.CloudSyncFunctional.Sync does not exist)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
)

- Fix FileClassifier to use exact token equality (Equals) not substring
  matching (Contains); add false-positive test for keyword-in-token case
- Fix SyncScheduler.TriggerAccountAsync to handle SyncAccountAsync Result:
  SyncCompleted fires on success only; failures are logged via LogSyncFailed
- Register IHttpClientFactory via AddHttpClient() in App DI composition
  root; GraphService and HttpDownloader both depend on it
- Thread accountId and driveId through ISyncWorker.ExecuteAsync and
  ISyncPipeline.RunAsync so uploads resolve the correct drive context
- Replace GetClientForToken throw-in-Match with TryGetClientForToken
  returning Result; callers use BindAsync
- Replace System.IO.* in GraphService.UploadFileAsync with IFileSystem
- Replace raw new HttpClient() in UploadChunksAsync with IHttpClientFactory
- Add full chunk retry loop to UploadChunksAsync (429, 404, backoff)
- Resolve GetDownloadUrlAsync before passing URL to IHttpDownloader
- Fix HttpDownloader.WriteFileAsync to use FileMode.Create (truncate)
- Fix HttpDownloader 429 double-sleep: single delay then continue
- Fix SyncPipeline to use Interlocked.Increment for global progress counter
- Fix SyncedItemRegistrar to match UpsertAsync result and log on failure
- Fix SyncService.ResolveConflictAsync to handle result in Skip branch
- Fix RemoteFolderEnumerator and LocalDeletionDetector intermediate
  unwrapping: chain MatchAsync directly on Task
- Introduce SyncRule domain record + SyncRuleFactory; remove SyncRuleEntity
  dependency from SyncRuleEvaluator and RemoteFolderEnumerator
- Add SyncConflictFactory; typed AccountId/OneDriveItemId on SyncConflict
- Remove primitive string? DriveId from OneDriveAccount; DriveIdValue only
- Extract MapToDomain helper in SyncScheduler to eliminate duplication
- Add interfaces for all six pipeline steps; SyncService injects by interface
- Add LocalChangeDetector: UploadJob carries ParentFolderPath (resolved at
  execution time by SyncWorker via GetFolderIdByPathAsync)
- Update GivenASyncConflict and GivenASyncRuleEvaluator to use factories

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@jaybarden1 jaybarden1 requested a review from a team May 28, 2026 19:12
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 28, 2026

Test results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 1e72313.

♻️ This comment has been updated with latest results.

@jaybarden1 jaybarden1 enabled auto-merge (squash) May 28, 2026 19:15
jaybarden1 and others added 4 commits May 28, 2026 20:56
Tests cover:
- AccountViewModel.AccountId property (missing)
- WorkspaceViewModel.OnWizardCompleted populates FolderNode from SelectedFolders (missing)
- WorkspaceViewModel.LoadPersistedAccounts loads sync rules as FolderNode (missing)
- WorkspaceViewModel.TriggerSync command calls ISyncScheduler (missing)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- SyncRuleEntity no longer leaks above the repository layer; ISyncRuleRepository.GetByAccountAsync now returns IReadOnlyList<SyncRule> with entity-to-domain mapping in the repository
- Add GetAllByAccountIdsAsync batch method to eliminate N+1 query in WorkspaceViewModel.LoadPersistedAccountsAsync
- WorkspaceViewModel exposes HasError/ErrorMessage reactive properties; TriggerSync.ThrownExceptions subscribed and logs via optional ILogger
- canSync predicate now gates on _syncScheduler is not null, preventing silent no-op when scheduler absent
- Extract InitializeCommands() to eliminate duplicated canSync/TriggerSync wiring across constructors
- MapToViewModel single-pass: build includedFolders list once, derive FolderCount from .Count
- ExecuteTriggerSyncAsync simplified to one-liner; canSync guards make the null checks redundant
- WorkspaceViewModel implements IDisposable; CompositeDisposable cleans up ThrownExceptions subscription
- SyncService removes redundant entity→domain mapping step now that the repository returns domain records

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix ODataError exceptions logging full error code/message instead of generic ex.Message
- Fix Linux token cache registration by adding LinuxKeyring attributes, with unprotected-file fallback
- Add GetDriveIdAsync to IGraphService/GraphService; wizard now fetches and persists DriveId on account creation
- Fix cross-thread InvalidOperationException from ReactiveCommandBase.OnCanExecuteChanged by adding outputScheduler: AvaloniaScheduler.Instance to all commands
- Replace Dispatcher.UIThread.Post with RxSchedulers.MainThreadScheduler.Schedule so property mutations are testable (ImmediateScheduler in tests, AvaloniaScheduler in production)
- Fix test fixture to set RxSchedulers.MainThreadScheduler = ImmediateScheduler.Instance; add GetDriveIdAsync mock to AddAccount tests; add test for GetDriveIdAsync failure path

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@jbarden jbarden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this went through several rounds of review and fix before it got to me...there are a few methods that are still too long etc but I know there's a plan to test out matt pocock's "improve-codebase-architecture" skill so will accept them to see if it flags

@jaybarden1 jaybarden1 merged commit 9562bf9 into main May 29, 2026
5 of 6 checks passed
@jaybarden1 jaybarden1 deleted the feature/27-sync-pipeline branch May 29, 2026 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants