feat!: add Git version control for editing projects - #2164
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Git-based project version control across repository services, lifecycle coordination, configuration, serialization, Avalonia UI, localization, and automated tests. It supports snapshots, history, restore, branches, remotes, Git discovery, scoped repositories, lock recovery, and asynchronous shutdown. ChangesProject Git version-control
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confidence Score: 5/5The pull request appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (60): Last reviewed commit: "fix(shell): keep the application open wh..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![drift-check[bot]](https://avatars.githubusercontent.com/in/4198242?s=60&v=4){kind=small}
There was a problem hiding this comment.
Documentation drift
Code Review Bot flagged 1 possible documentation drift(s). These are advisory.
README.md(info):The code introduces a new 'Version Control' feature with the 'VersionControlTabExtension' and Git integration strings in 'Strings.resx', but it is not listed in the 'Features' section of 'README.md'.— suggested:Add a 'Version Control' section to the 'Features' part of the README.md, mentioning that the software now supports tracking project history using Git.
Results are for commit ac1399e. On newer commits, the bot's summary comment reflects the latest run.
Code Review BotNo reviewable code changes were analyzed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (21)
tests/Beutl.HeadlessUITests/VersionControlConflictTests.cs-36-44 (1)
36-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCapture the pre-load state instead of asserting inside the callback.
Assert.Thatat Line 38 runs inside the warning callback, which the productionOpenProjectpath invokes. If the assertion fails, theAssertionExceptiontravels throughOpenProject. If that path catches exceptions and reports them as a notification, the failure never reaches NUnit and the test passes silently. Record the observed state and assert it afterOpenProjectreturns.🧪 Proposed change
string? warnedFile = null; + bool projectWasNullAtWarning = false; TestShell.VersionControl.WarnConflictMarkersAsync = file => { - Assert.That( - TestShell.Project.CurrentProject.Value, - Is.Null, - "the warning must run before project loading starts"); + projectWasNullAtWarning = TestShell.Project.CurrentProject.Value is null; warnedFile = file; return Task.CompletedTask; };Then assert
projectWasNullAtWarninginside the existingAssert.Multipleblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/VersionControlConflictTests.cs` around lines 36 - 44, Update the WarnConflictMarkersAsync callback in VersionControlConflictTests to record whether TestShell.Project.CurrentProject.Value is null in a projectWasNullAtWarning variable instead of asserting there. After OpenProject returns, assert that captured value within the existing Assert.Multiple block, alongside the other test assertions.tests/Beutl.HeadlessUITests/VersionControlTabViewTests.cs-106-113 (1)
106-113: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThis negative assertion can flake.
RunGitcreates theflyout-refreshbranch outside the application. The assertion at Lines 111-113 then requires that the view model has not observed the branch yet. If the repository watcher refreshes branches between the two calls, the assertion fails. Assert the refresh-on-open behavior only through the positive check afterShowAtat Lines 116 and 138-140, or make the pre-condition tolerant of an early refresh.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/VersionControlTabViewTests.cs` around lines 106 - 113, Remove the negative assertion on `viewModel.Branches` immediately after creating `flyout-refresh` with `RunGit`, since the watcher may refresh before the assertion. Keep validation focused on the positive branch-presence check after `ShowAt`, or make any retained precondition allow the branch to already be present.tests/Beutl.UnitTests/Editor/VersionControl/VersionControlTabViewModelTests.cs-1137-1140 (1)
1137-1140: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace the fixed delay with an explicit start signal.
await Task.Delay(20)assumes the pull reaches the coordinator within 20 ms. Under CI load the cancel can run before the operation starts, and the status assertion then fails. Signal the start from the coordinator setup, as the tests at Lines 677-693 and 768-786 do.🔧 Proposed fix
var coordinator = new Mock<IProjectVersionControlCoordinator>(); + var pullStarted = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); coordinator.Setup(x => x.PullAsync(It.IsAny<CancellationToken>())) .Returns<CancellationToken>(async cancellationToken => { + pullStarted.TrySetResult(); await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken); return new RemoteOpResult.Success(); });Task pull = viewModel.PullAsync(); - await Task.Delay(20); + await pullStarted.Task; viewModel.CancelRemoteOperationCommand.Execute(); await pull;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.UnitTests/Editor/VersionControl/VersionControlTabViewModelTests.cs` around lines 1137 - 1140, Replace the fixed Task.Delay in the PullAsync cancellation test with an explicit start signal emitted by the coordinator setup, following the synchronization pattern used by the nearby tests around lines 677-693 and 768-786. Await that signal before executing CancelRemoteOperationCommand, then preserve the existing await and status assertions.tests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherStressTests.cs-104-114 (1)
104-114: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRead
GIT_OPTIONAL_LOCKSwithTryGetValue.The indexer throws
KeyNotFoundExceptionif the variable is absent. In that case the test fails with an unrelated exception instead of the assertion message at Line 51, and_statusCallsWithoutOptionalLocksis never incremented. A missing variable is the same defect as a wrong value.🐛 Proposed fix
if (arguments.FirstOrDefault() == "status") { Interlocked.Increment(ref _statusCallCount); - string? optionalLocks = inner - .CreateStartInfo(repository, arguments, networkOperation) - .Environment["GIT_OPTIONAL_LOCKS"]; - if (optionalLocks != "0") + bool found = inner + .CreateStartInfo(repository, arguments, networkOperation) + .Environment + .TryGetValue("GIT_OPTIONAL_LOCKS", out string? optionalLocks); + if (!found || optionalLocks != "0") { Interlocked.Increment(ref _statusCallsWithoutOptionalLocks); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherStressTests.cs` around lines 104 - 114, Update the environment-variable lookup in the status branch of the repository watcher test to use TryGetValue instead of the indexer. Treat both a missing GIT_OPTIONAL_LOCKS entry and any value other than "0" as a call without optional locks, incrementing _statusCallsWithoutOptionalLocks in either case.src/Beutl/ViewModels/MenuBarViewModel.cs-18-21 (1)
18-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the public constructor change as breaking.
MenuBarViewModelis public, and the constructor now requires a third parameter,versionControlCoordinator. External callers that use the two-parameter constructor no longer compile. Use afeat!:orrefactor!:commit and add aBREAKING CHANGE:footer for this change.Based on coding guidelines: "Breaking public-surface removals or changes must use
refactor!:orfeat!:and include aBREAKING CHANGE:footer".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/ViewModels/MenuBarViewModel.cs` around lines 18 - 21, Mark the commit introducing the versionControlCoordinator parameter in MenuBarViewModel’s public constructor as breaking by using a feat! or refactor! type and adding a BREAKING CHANGE: footer describing that two-parameter constructor callers must now provide VersionControlCoordinator.Source: Coding guidelines
src/Beutl/Services/VersionControlCoordinator.cs-284-310 (1)
284-310: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPrevent disposed access to
_isGitAvailableand_isTracked.
Disposedisposes these values without awaiting the activation/availability paths.ActivateRepositoryAsynccan callReplaceServiceafter disposal, andReplaceServicewrites_isTracked.Valuewithout a_disposedcheck.GetAvailabilityAsyncalso writes_isGitAvailable.Valueafter anawaitonly after a non-atomic disposed check. Add a_disposedguard inReplaceServicebefore_isTracked.Value = ..., and add a disposed guard inGetAvailabilityAsyncbefore writing_isGitAvailable.Value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Services/VersionControlCoordinator.cs` around lines 284 - 310, Add a _disposed guard in ReplaceService immediately before writing _isTracked.Value, and add a corresponding guard in GetAvailabilityAsync after its await and immediately before writing _isGitAvailable.Value. Ensure both asynchronous paths return without accessing these disposed values once disposal has begun.src/Beutl.Editor.Components/VersionControl/ViewModels/TitleBarBranchViewModel.cs-356-370 (1)
356-370: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe reset closure cannot run when the coordinator reports Git as unavailable.
Lines 360-368 post
ResetRepositoryStateonly ifIsCurrentService(...)returns true.IsCurrentServicerequires_coordinatorGitAvailable(line 408). If this branch is entered because!_coordinatorGitAvailable, the guard always fails, and the branch UI state is never reset here. Split the guard so the reset does not depend on_coordinatorGitAvailable.🐛 Proposed fix
_postToUi(() => { - if (IsCurrentService(service, revision, cancellationToken)) + if (!_disposed + && !cancellationToken.IsCancellationRequested + && revision == _serviceRevision + && ReferenceEquals(service, _service)) { _gitAvailable = availability.State == GitAvailabilityState.Installed; ResetRepositoryState(); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor.Components/VersionControl/ViewModels/TitleBarBranchViewModel.cs` around lines 356 - 370, Update the unavailable-Git branch around IsCurrentService so ResetRepositoryState can execute when _coordinatorGitAvailable is false. Split the current guard or use a service/revision validity check that excludes the coordinator-availability requirement, while preserving the existing _gitAvailable assignment and stale-operation protection.tests/Beutl.HeadlessUITests/CreateNewProjectDialogTests.cs-42-66 (1)
42-66: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake shell-state cleanup deterministic.
These tests use global shell state through
ProjectServiceandTestShell.
tests/Beutl.HeadlessUITests/CreateNewProjectDialogTests.cs#L42-L66: reset the shell at the start of the test body before constructingProjectService.tests/Beutl.HeadlessUITests/CreateNewProjectDialogTests.cs#L68-L89: move the final reset into afinallyblock so an assertion failure cannot leave an open project for later tests.As per coding guidelines, “Shell tests in
tests/Beutl.HeadlessUITests/that touch global singletons such asProjectService.CurrentorEditorService.Currentmust reset them at the start of each[AvaloniaTest]body.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/CreateNewProjectDialogTests.cs` around lines 42 - 66, The test Track_history_uses_the_configured_default_and_is_present_in_the_dialog must reset TestShell at the start before constructing ProjectService. In CreateNewProjectDialogTests.cs lines 68-89, move the final shell reset into a finally block so cleanup runs even when assertions fail; apply the corresponding cleanup changes at both listed sites.Source: Coding guidelines
src/Beutl/ViewModels/Dialogs/CreateNewProjectViewModel.cs-105-117 (1)
105-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh Git availability before history initialization.
DetectGitAsyncruns without awaiting, andIsGitAvailablestarts as false. If the user selects history and creates the project before detection completes, this condition skips initialization permanently even when Git is installed. Await availability in the create flow, or disable creation until detection completes.Proposed fix
- if (project is not null - && TrackHistory.Value - && IsGitAvailable.Value - && _versionControlCoordinator is not null - && _requestIdentityAsync is not null) + if (project is not null + && TrackHistory.Value + && _versionControlCoordinator is not null + && _requestIdentityAsync is not null) { - await _versionControlCoordinator.InitializeCurrentProjectAsync(_requestIdentityAsync); + GitAvailability availability = await _versionControlCoordinator.GetAvailabilityAsync(); + IsGitAvailable.Value = availability.State == GitAvailabilityState.Installed; + if (IsGitAvailable.Value) + { + await _versionControlCoordinator.InitializeCurrentProjectAsync(_requestIdentityAsync); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/ViewModels/Dialogs/CreateNewProjectViewModel.cs` around lines 105 - 117, Ensure Git detection has completed before the history initialization condition in the project creation flow, such as by awaiting DetectGitAsync immediately before checking IsGitAvailable. Preserve the existing TrackHistory, Git availability, coordinator, and request identity guards when calling InitializeCurrentProjectAsync.src/Beutl.Editor.Components/VersionControlTab/ViewModels/VersionControlTabViewModel.cs-594-627 (1)
594-627: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDispose
_historyGate.
_historyGateis aSemaphoreSlimcreated at Line 26.Disposenever releases it, and it is not registered in_disposables. The catch clauses at Lines 384-387 and 396-398 already expectObjectDisposedException, so disposal is the intended lifetime.♻️ Proposed fix
Commits.Clear(); ChangedFiles.Clear(); DiffLines.Clear(); IsSelected.Dispose(); _disposables.Dispose(); + _historyGate.Dispose(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor.Components/VersionControlTab/ViewModels/VersionControlTabViewModel.cs` around lines 594 - 627, Update Dispose in VersionControlTabViewModel to dispose the _historyGate SemaphoreSlim during teardown, alongside the other cancellation and disposable resources. Keep disposal idempotent by performing it only after the existing _disposed guard, and do not rely on _disposables because _historyGate is not registered there.src/Beutl/Views/MainView.axaml-149-155 (1)
149-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate the Commit command on project state, like Enable Version Control. Both windows add a "Project" menu where
EnableVersionControlis disabled viaIsEnabled="{CompiledBinding IsProjectOpened.Value}"butCommitVersionhas no such gating. A user can invoke Commit with no project open;CommitVersionAsync(insrc/Beutl/Views/MainView.axaml.InitializeMenuBar.cs) prompts for a commit message before any project-state check, so the failure only surfaces after the user types a message and confirms.
src/Beutl/Views/MainView.axaml#L149-L155: addIsEnabled="{CompiledBinding IsProjectOpened.Value}"to theCommitVersionMenuItem, matchingEnableVersionControl.src/Beutl/Views/MacWindow.axaml#L100-L108: add the sameIsEnabled="{CompiledBinding IsProjectOpened.Value}"to theCommitVersionNativeMenuItem.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Views/MainView.axaml` around lines 149 - 155, Gate the CommitVersion command on IsProjectOpened.Value, matching EnableVersionControl. Update src/Beutl/Views/MainView.axaml lines 149-155 and src/Beutl/Views/MacWindow.axaml lines 100-108 by adding the same compiled IsEnabled binding to each CommitVersion menu item.src/Beutl/Views/MainView.axaml.InitializeMenuBar.cs-77-94 (1)
77-94: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNotify the user when Git is unavailable.
EnableVersionControlAsyncreturns silently whenavailability.State != GitAvailabilityState.Installed. The user clicks "Enable Version Control" and observes no feedback at all. Show a notification that explains why version control could not be enabled.🔔 Proposed fix to surface the unavailable state
GitAvailability availability = await viewModel.VersionControlCoordinator.GetAvailabilityAsync(); if (availability.State != GitAvailabilityState.Installed) { + NotificationService.ShowWarning( + Strings.VersionControl, + Strings.VersionControl_GitUnavailable); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Views/MainView.axaml.InitializeMenuBar.cs` around lines 77 - 94, Update EnableVersionControlAsync to notify the user when GetAvailabilityAsync returns a GitAvailability state other than Installed, before returning. Use the existing notification mechanism and include a clear message explaining that version control cannot be enabled because Git is unavailable; leave the installed and exception-handling paths unchanged.src/Beutl/Views/TitleBarBranchView.axaml.cs-27-48 (1)
27-48: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle failures in the branch flyout handlers.
OnBranchFlyoutOpeningcallsPrepareFlyoutAsync(), which awaitsRefreshAsync()and can propagate exceptions fromservice.GetAvailabilityAsync(),GetStatusAsync(), orGetBranchesAsync()through thisasync voidhandler.SwitchBranchAsyncalso contains operation failure paths that can surface from the UI event path; wrap both handlers with the version-control error handling pattern so failures cannot crash the UI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Views/TitleBarBranchView.axaml.cs` around lines 27 - 48, Wrap the awaited operations in OnBranchFlyoutOpening and OnBranchClick with the existing version-control error-handling pattern used by the surrounding code. Handle failures from PrepareFlyoutAsync and SwitchBranchAsync within these async void handlers so exceptions from refresh or branch switching do not escape into the UI event loop.docs/specs/005-project-git-versioning/tasks.md-34-34 (1)
34-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the T006 call-site count.
The task says “five GUI call sites”, but the listed locations contain six calls: 2 + 1 + 2 + 1. Correct the count or the list to prevent an omitted migration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/005-project-git-versioning/tasks.md` at line 34, Correct T006’s call-site count to six, matching the listed RandomFileNameGenerator usages across ElementAdderImpl, ElementStructureService, ElementClipboardService, and DuplicateHelper; preserve the complete location list so no migration is omitted.docs/specs/005-project-git-versioning/contracts/version-control-service.md-42-42 (1)
42-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the second restore pass.
RestoreWorktreeFromCoreAsyncrunsrestore→clean -fd→restoreagain (seesrc/Beutl.Editor/VersionControl/GitCliVersionControlService.cslines 738-756). The extra pass reapplies paths thatcleanremoves because a worktree-only restore leaves them untracked. Record that step here so the contract matches the implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/005-project-git-versioning/contracts/version-control-service.md` at line 42, Update the RestoreWorktreeFromAsync contract to document the full restore sequence: run restore, clean -fd, then run restore again. Preserve the existing project-closed precondition and ignored-file behavior while noting that the second restore reapplies paths removed as untracked by clean.src/Beutl.Editor/VersionControl/GitCliRunner.cs-195-196 (1)
195-196: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle a failed lock file deletion.
File.DeletethrowsIOExceptionorUnauthorizedAccessExceptionwhen another process holdsindex.lockor when permissions deny removal. The method signature returnsbool, and the callerGitCliVersionControlService.RemoveRecoverableLockAsynctreatsfalseas "not removed". Convert the failure tofalseso the UI reports a normal negative result instead of an unhandled exception.🛡️ Proposed fix
- File.Delete(current.LockPath); - return true; + try + { + File.Delete(current.LockPath); + return true; + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + return false; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor/VersionControl/GitCliRunner.cs` around lines 195 - 196, Update the lock-removal method containing File.Delete(current.LockPath) to catch IOException and UnauthorizedAccessException, return false when deletion fails, and retain the existing true result after successful deletion so RemoveRecoverableLockAsync receives the expected boolean outcome.src/Beutl.Editor/VersionControl/GitInstallationLocator.cs-249-257 (1)
249-257: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the kill call against a race.
The process can exit between
process.HasExitedandprocess.Kill.Killthen throwsInvalidOperationExceptionorWin32Exception, which replaces theOperationCanceledExceptionthat the caller expects.GitCliRunner.TryKillProcessTreealready swallows those exception types. Apply the same guard here.🛡️ Proposed fix
catch (OperationCanceledException) { - if (!process.HasExited) - { - process.Kill(entireProcessTree: true); - } - + try + { + if (!process.HasExited) + { + process.Kill(entireProcessTree: true); + } + } + catch (Exception ex) when (ex is InvalidOperationException + or System.ComponentModel.Win32Exception + or NotSupportedException) + { + } + throw; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor/VersionControl/GitInstallationLocator.cs` around lines 249 - 257, Update the OperationCanceledException handler in the process execution flow to guard process.Kill against the exit race, matching the exception-swallowing behavior of GitCliRunner.TryKillProcessTree. Ensure InvalidOperationException and Win32Exception from killing an already-exited process are suppressed so the original OperationCanceledException is rethrown.src/Beutl.Editor/VersionControl/RepositoryWatcher.cs-102-109 (1)
102-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEvaluate the old path on rename events.
OnFileSystemChangedreads onlye.FullPath. For a rename, that is the new path. When a tracked file is renamed to an excluded name, for examplescene.scenetoscene.tmp,ShouldExcludePathsuppresses the notification even though the repository status changed. HandleRenamedEventArgs.OldFullPathas well.🐛 Proposed fix
- _watcher.Renamed += OnFileSystemChanged; + _watcher.Renamed += OnFileSystemRenamed; _watcher.Error += OnWatcherError; } private void OnFileSystemChanged(object sender, FileSystemEventArgs e) { NotifyPathChanged(e.FullPath); } + + private void OnFileSystemRenamed(object sender, RenamedEventArgs e) + { + NotifyPathChanged(e.OldFullPath); + NotifyPathChanged(e.FullPath); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor/VersionControl/RepositoryWatcher.cs` around lines 102 - 109, Update OnFileSystemChanged to detect RenamedEventArgs and evaluate both FullPath and OldFullPath through the existing path-change notification flow. Preserve the current handling for non-rename FileSystemEventArgs while ensuring renames into or out of excluded paths still notify the repository watcher.src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs-1762-1778 (1)
1762-1778: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCatch and log every failure on the watcher refresh path.
OnRepositoryChangedstarts this task with_ = RefreshStatusFromWatcherAsync()(line 1715), so nothing observes the returned task. The method catchesGitOperationExceptionandInvalidOperationExceptiononly.GitCliRunner.RunAsyncalso throwsTimeoutExceptionwhen a local Git command exceeds_localTimeout, andIOExceptioncan escape the lock-path probe. Those escape into an unobserved task, and the failure leaves no trace.Catch the remaining exceptions and log them so a repeatedly failing
git statusis diagnosable.🛡️ Proposed fix
catch (GitOperationException) { } catch (InvalidOperationException) { } + catch (Exception) + { + // The watcher refresh is fire-and-forget; never let a failure escape unobserved. + }Prefer a logged variant if this project has a logger available in
Beutl.Editor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs` around lines 1762 - 1778, Update RefreshStatusFromWatcherAsync to catch TimeoutException and IOException in addition to the existing failure cases, and log every caught failure using the logger already available in Beutl.Editor. Preserve the silent handling for disposal and expected Git/operation failures as appropriate, while ensuring unobserved watcher-task exceptions leave a diagnostic trace.src/Beutl.Editor/VersionControl/GitCliRunner.cs-298-312 (1)
298-312: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winProtect the lock-path probe against I/O failures.
File.ReadAllText(dotGitPath)can throwIOExceptionorUnauthorizedAccessException.GetRecoverableRepositoryLockdoes not catch these.GitCliVersionControlService.CaptureRecoverableLockcalls it from inside thecatch (GitOperationException)block ofRunSerializedAsync, so an I/O failure here replaces the original Git error and the real cause is lost. Returnnullon a failed read.🛡️ Proposed fix
if (File.Exists(dotGitPath)) { const string prefix = "gitdir:"; - string contents = File.ReadAllText(dotGitPath).Trim(); + string contents; + try + { + contents = File.ReadAllText(dotGitPath).Trim(); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + return Path.Combine(dotGitPath, "index.lock"); + } + if (contents.StartsWith(prefix, StringComparison.OrdinalIgnoreCase))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor/VersionControl/GitCliRunner.cs` around lines 298 - 312, Update GetRecoverableRepositoryLock so the File.ReadAllText(dotGitPath) probe catches IOException and UnauthorizedAccessException and returns null when reading fails. Preserve the existing path resolution and index.lock return behavior for successful reads, ensuring CaptureRecoverableLock does not replace the original GitOperationException.docs/specs/005-project-git-versioning/contracts/version-control-service.md-21-31 (1)
21-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the contract to match the implemented interface.
The documented seam differs from
src/Beutl.Editor/VersionControl/IProjectVersionControlService.cs:
CreateBranchAsynctakes an additionalstring startPointparameter in the code.SetLocalIdentityAsynctakes aGitIdentity identityin the code, not(string name, string email).- The code also declares
DiscoverRepositoryAsync(string projectRoot, …), which this listing omits.Align the contract document so plugin authors read the real surface.
📝 Proposed doc fix
Task<IReadOnlyList<BranchInfo>> GetBranchesAsync(CancellationToken ct); - Task CreateBranchAsync(string name, CancellationToken ct); + Task CreateBranchAsync(string name, string startPoint, CancellationToken ct); Task SwitchBranchAsync(string name, CancellationToken ct); // low-level; cycle orchestrated above Task RestoreWorktreeFromAsync(string sha, CancellationToken ct); // restore + clean, pathspec-scoped @@ Task<GitIdentity?> GetIdentityAsync(CancellationToken ct); - Task SetLocalIdentityAsync(string name, string email, CancellationToken ct); // repo-local only + Task SetLocalIdentityAsync(GitIdentity identity, CancellationToken ct); // repo-local onlyAlso add
Task<RepositoryInfo?> DiscoverRepositoryAsync(string projectRoot, CancellationToken ct);nearInitializeAsync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/005-project-git-versioning/contracts/version-control-service.md` around lines 21 - 31, Update the documented interface near InitializeAsync to include DiscoverRepositoryAsync(string projectRoot, CancellationToken ct), change CreateBranchAsync to accept the implemented startPoint parameter, and change SetLocalIdentityAsync to accept a GitIdentity identity instead of separate name and email strings. Keep the remaining contract signatures unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/specs/005-project-git-versioning/research.md`:
- Around line 118-120: Align the enclosing-repository contract across the
documentation: in docs/specs/005-project-git-versioning/research.md lines
118-120, retain whole-repository behavior for branch switching, pull, and push
and document the required UI disclosure and guardrails; in quickstart.md line
11, state that only pathspec-compatible operations are project-scoped; in
spec.md lines 98-99, remove the promise that all operations are project-scoped;
in spec.md lines 156-157, limit the unrelated-files guarantee to operations
supporting pathspecs; and in tasks.md line 87, add tests covering branch, pull,
and push behavior within an enclosing repository.
- Line 30: Document a safe SSH command precedence rule in
docs/specs/005-project-git-versioning/research.md:30 that preserves user SSH
wrappers, proxy settings, key arguments, agents, and core.sshCommand instead of
unconditionally setting GIT_SSH_COMMAND. In
docs/specs/005-project-git-versioning/quickstart.md:35 and
docs/specs/005-project-git-versioning/spec.md:146-149, retain the
existing-authentication delegation wording only when user SSH configuration
remains effective. Add manual or test coverage for custom wrappers, agents,
proxies, and configured SSH commands across these authentication flows.
- Line 45: Update the Git initialization flow described in
docs/specs/005-project-git-versioning/research.md:45 and
docs/specs/005-project-git-versioning/tasks.md:51-52 to support Git 2.23–2.27 by
using git init followed by git symbolic-ref HEAD refs/heads/main instead of
relying on git init -b main for those versions. Retain the declared 2.23+
minimum and add tests covering the fallback initialization path.
In `@docs/specs/005-project-git-versioning/spec.md`:
- Line 264: Update SC-004 in the project versioning specification so restore,
branch-switch, and pull flows require recording a reachable safety version only
when the worktree is dirty; preserve the guarantees against losing committed
work or the currently saved project state, while allowing clean flows without
creating empty versions.
In
`@src/Beutl.Editor.Components/VersionControlTab/Views/VersionControlChangesView.axaml.cs`:
- Around line 14-45: Introduce one shared exception boundary for the async void
handlers in
src/Beutl.Editor.Components/VersionControlTab/Views/VersionControlChangesView.axaml.cs
lines 14-45, wrapping the awaits in OnChangedFileSelectionChanged,
OnRestoreClick, and OnRestoreToNewBranchClick and reporting failures through the
existing notification service. Apply the same boundary to
OnCommitSelectionChanged in
src/Beutl.Editor.Components/VersionControlTab/Views/VersionControlHistoryView.axaml.cs
lines 19-43, preserving cancellation handling while preventing other Git
operation exceptions from escaping to the Avalonia dispatcher.
In `@src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs`:
- Around line 403-418: Make disposal state in Dispose and its readers
thread-safe by replacing the plain _disposed check-then-set with an atomic
Interlocked.Exchange-based guard (or an equivalent volatile implementation),
ensuring concurrent disposal is idempotent and no worker observes stale state.
Dispose _operationGate only after confirming every RunSerializedAsync caller
handles ObjectDisposedException from WaitAsync as normal shutdown; otherwise
leave the gate undisposed.
In `@src/Beutl.Editor/VersionControl/VersionControlModels.cs`:
- Around line 23-58: Update RepositoryInfo.Equals(RepositoryInfo?) and
GetHashCode() to compare RepoRoot and ProjectRoot using the platform-aware
PathComparison rule, matching constructor normalization and Windows
case-insensitivity. In src/Beutl.Editor/VersionControl/VersionControlModels.cs
lines 23-58, implement the equality and hash changes; in
src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs lines 893-911,
verify both Equals(discoveredRepository, options.TargetRepository) checks no
longer trigger EnclosingRepositoryConsentRequiredException or
InvalidOperationException when path casing differs, with no direct change
required there unless verification exposes one.
- Around line 169-178: Redact embedded URL credentials from Git stderr before
exposing it. Add a RedactCredentials helper near CreateMessage that replaces URL
userinfo in the form ://<userinfo>@ with ://***@, use it when constructing the
message in CreateMessage, and apply it to the stderr payload returned by
GitCliVersionControlService.MapRemoteFailure.
In `@src/Beutl/ViewModels/EditContext/ElementAdderImpl.cs`:
- Around line 45-52: Guard the AddElement and AddElementFromTemplate flows
before element creation or URI derivation when the scene has no saved URI.
Report the existing save-required condition through the command’s established
mechanism, and only call ElementFileNaming.GetUri after confirming scene.Uri is
non-null; remove the null-forgiving assumption while preserving normal
saved-scene behavior.
In `@src/Beutl/ViewModels/MainViewModel.cs`:
- Around line 163-167: Update VersionControlCoordinator.NotifyClosingAsync to
provide a bounded cancellation-token overload, using a timeout-backed
CancellationTokenSource while preserving the existing cancellation behavior. In
MainViewModel.Dispose(), call this bounded overload instead of synchronously
waiting on the unbounded NotifyClosingAsync task, so shutdown proceeds when Git
operations hang.
In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs`:
- Around line 49-56: Capture the existing TestShell.VersionControl callbacks
before overriding them, then restore each in the corresponding finally block:
RequestIdentityAsync in the test around its assignment, and likewise
ConfirmSwitchBranchAsync, ConfirmPullAsync, and ConfirmRestoreAsync. Preserve
the current test behavior while ensuring all process-global coordinator
delegates are returned to their previous values after each test.
---
Minor comments:
In `@docs/specs/005-project-git-versioning/contracts/version-control-service.md`:
- Line 42: Update the RestoreWorktreeFromAsync contract to document the full
restore sequence: run restore, clean -fd, then run restore again. Preserve the
existing project-closed precondition and ignored-file behavior while noting that
the second restore reapplies paths removed as untracked by clean.
- Around line 21-31: Update the documented interface near InitializeAsync to
include DiscoverRepositoryAsync(string projectRoot, CancellationToken ct),
change CreateBranchAsync to accept the implemented startPoint parameter, and
change SetLocalIdentityAsync to accept a GitIdentity identity instead of
separate name and email strings. Keep the remaining contract signatures
unchanged.
In `@docs/specs/005-project-git-versioning/tasks.md`:
- Line 34: Correct T006’s call-site count to six, matching the listed
RandomFileNameGenerator usages across ElementAdderImpl, ElementStructureService,
ElementClipboardService, and DuplicateHelper; preserve the complete location
list so no migration is omitted.
In
`@src/Beutl.Editor.Components/VersionControl/ViewModels/TitleBarBranchViewModel.cs`:
- Around line 356-370: Update the unavailable-Git branch around IsCurrentService
so ResetRepositoryState can execute when _coordinatorGitAvailable is false.
Split the current guard or use a service/revision validity check that excludes
the coordinator-availability requirement, while preserving the existing
_gitAvailable assignment and stale-operation protection.
In
`@src/Beutl.Editor.Components/VersionControlTab/ViewModels/VersionControlTabViewModel.cs`:
- Around line 594-627: Update Dispose in VersionControlTabViewModel to dispose
the _historyGate SemaphoreSlim during teardown, alongside the other cancellation
and disposable resources. Keep disposal idempotent by performing it only after
the existing _disposed guard, and do not rely on _disposables because
_historyGate is not registered there.
In `@src/Beutl.Editor/VersionControl/GitCliRunner.cs`:
- Around line 195-196: Update the lock-removal method containing
File.Delete(current.LockPath) to catch IOException and
UnauthorizedAccessException, return false when deletion fails, and retain the
existing true result after successful deletion so RemoveRecoverableLockAsync
receives the expected boolean outcome.
- Around line 298-312: Update GetRecoverableRepositoryLock so the
File.ReadAllText(dotGitPath) probe catches IOException and
UnauthorizedAccessException and returns null when reading fails. Preserve the
existing path resolution and index.lock return behavior for successful reads,
ensuring CaptureRecoverableLock does not replace the original
GitOperationException.
In `@src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs`:
- Around line 1762-1778: Update RefreshStatusFromWatcherAsync to catch
TimeoutException and IOException in addition to the existing failure cases, and
log every caught failure using the logger already available in Beutl.Editor.
Preserve the silent handling for disposal and expected Git/operation failures as
appropriate, while ensuring unobserved watcher-task exceptions leave a
diagnostic trace.
In `@src/Beutl.Editor/VersionControl/GitInstallationLocator.cs`:
- Around line 249-257: Update the OperationCanceledException handler in the
process execution flow to guard process.Kill against the exit race, matching the
exception-swallowing behavior of GitCliRunner.TryKillProcessTree. Ensure
InvalidOperationException and Win32Exception from killing an already-exited
process are suppressed so the original OperationCanceledException is rethrown.
In `@src/Beutl.Editor/VersionControl/RepositoryWatcher.cs`:
- Around line 102-109: Update OnFileSystemChanged to detect RenamedEventArgs and
evaluate both FullPath and OldFullPath through the existing path-change
notification flow. Preserve the current handling for non-rename
FileSystemEventArgs while ensuring renames into or out of excluded paths still
notify the repository watcher.
In `@src/Beutl/Services/VersionControlCoordinator.cs`:
- Around line 284-310: Add a _disposed guard in ReplaceService immediately
before writing _isTracked.Value, and add a corresponding guard in
GetAvailabilityAsync after its await and immediately before writing
_isGitAvailable.Value. Ensure both asynchronous paths return without accessing
these disposed values once disposal has begun.
In `@src/Beutl/ViewModels/Dialogs/CreateNewProjectViewModel.cs`:
- Around line 105-117: Ensure Git detection has completed before the history
initialization condition in the project creation flow, such as by awaiting
DetectGitAsync immediately before checking IsGitAvailable. Preserve the existing
TrackHistory, Git availability, coordinator, and request identity guards when
calling InitializeCurrentProjectAsync.
In `@src/Beutl/ViewModels/MenuBarViewModel.cs`:
- Around line 18-21: Mark the commit introducing the versionControlCoordinator
parameter in MenuBarViewModel’s public constructor as breaking by using a feat!
or refactor! type and adding a BREAKING CHANGE: footer describing that
two-parameter constructor callers must now provide VersionControlCoordinator.
In `@src/Beutl/Views/MainView.axaml`:
- Around line 149-155: Gate the CommitVersion command on IsProjectOpened.Value,
matching EnableVersionControl. Update src/Beutl/Views/MainView.axaml lines
149-155 and src/Beutl/Views/MacWindow.axaml lines 100-108 by adding the same
compiled IsEnabled binding to each CommitVersion menu item.
In `@src/Beutl/Views/MainView.axaml.InitializeMenuBar.cs`:
- Around line 77-94: Update EnableVersionControlAsync to notify the user when
GetAvailabilityAsync returns a GitAvailability state other than Installed,
before returning. Use the existing notification mechanism and include a clear
message explaining that version control cannot be enabled because Git is
unavailable; leave the installed and exception-handling paths unchanged.
In `@src/Beutl/Views/TitleBarBranchView.axaml.cs`:
- Around line 27-48: Wrap the awaited operations in OnBranchFlyoutOpening and
OnBranchClick with the existing version-control error-handling pattern used by
the surrounding code. Handle failures from PrepareFlyoutAsync and
SwitchBranchAsync within these async void handlers so exceptions from refresh or
branch switching do not escape into the UI event loop.
In `@tests/Beutl.HeadlessUITests/CreateNewProjectDialogTests.cs`:
- Around line 42-66: The test
Track_history_uses_the_configured_default_and_is_present_in_the_dialog must
reset TestShell at the start before constructing ProjectService. In
CreateNewProjectDialogTests.cs lines 68-89, move the final shell reset into a
finally block so cleanup runs even when assertions fail; apply the corresponding
cleanup changes at both listed sites.
In `@tests/Beutl.HeadlessUITests/VersionControlConflictTests.cs`:
- Around line 36-44: Update the WarnConflictMarkersAsync callback in
VersionControlConflictTests to record whether
TestShell.Project.CurrentProject.Value is null in a projectWasNullAtWarning
variable instead of asserting there. After OpenProject returns, assert that
captured value within the existing Assert.Multiple block, alongside the other
test assertions.
In `@tests/Beutl.HeadlessUITests/VersionControlTabViewTests.cs`:
- Around line 106-113: Remove the negative assertion on `viewModel.Branches`
immediately after creating `flyout-refresh` with `RunGit`, since the watcher may
refresh before the assertion. Keep validation focused on the positive
branch-presence check after `ShowAt`, or make any retained precondition allow
the branch to already be present.
In `@tests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherStressTests.cs`:
- Around line 104-114: Update the environment-variable lookup in the status
branch of the repository watcher test to use TryGetValue instead of the indexer.
Treat both a missing GIT_OPTIONAL_LOCKS entry and any value other than "0" as a
call without optional locks, incrementing _statusCallsWithoutOptionalLocks in
either case.
In
`@tests/Beutl.UnitTests/Editor/VersionControl/VersionControlTabViewModelTests.cs`:
- Around line 1137-1140: Replace the fixed Task.Delay in the PullAsync
cancellation test with an explicit start signal emitted by the coordinator
setup, following the synchronization pattern used by the nearby tests around
lines 677-693 and 768-786. Await that signal before executing
CancelRemoteOperationCommand, then preserve the existing await and status
assertions.
---
Nitpick comments:
In
`@src/Beutl.Editor.Components/VersionControlTab/ViewModels/VersionControlTabViewModel.cs`:
- Around line 93-95: Remove the _lockRecoveryService-dependent initializer from
HasRecoverableLock in the VersionControlTabViewModel constructor, using a
neutral initial value until ResetRepositoryState assigns the actual repository
state. Keep the existing ResetRepositoryState update behavior unchanged.
- Around line 1315-1377: Replace the string-key lookup in
VersionControlRelativeTimeFormatter.GetString and its callers with the generated
Strings members, using a small switch for the plural resource selection while
preserving the existing culture-aware formatting. Remove the local
ResourceManager and MissingManifestResourceException path unless runtime culture
must differ from CultureInfo.CurrentUICulture; if it does, retain
ResourceManager and the existing localization coverage for every key.
In
`@src/Beutl.Editor.Components/VersionControlTab/Views/VersionControlTabView.axaml.cs`:
- Around line 89-90: Update the key check in the surrounding shortcut handler to
use a single Enter/Return member because Avalonia treats Key.Enter and
Key.Return as the same value; keep the condition clear while preserving the
existing modifier validation.
In `@src/Beutl.Editor/VersionControl/GitCliRunner.cs`:
- Around line 86-108: Update RunAsync immediately after process.Start() to close
the redirected standard input stream, ensuring Git receives EOF before output
tasks and timeout handling begin. Use the Process.StandardInput symbol and
preserve the existing process lifecycle and cancellation behavior.
In `@src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs`:
- Around line 1091-1113: Update CommitAllCoreAsync’s commit argument
construction to append the pathspec separator and repository.Pathspec after the
commit message arguments, ensuring git commit is scoped to the project path.
Preserve the existing snapshot-kind message handling and RunAsync invocation.
In `@src/Beutl.Editor/VersionControl/GitInstallationLocator.cs`:
- Around line 120-127: Extend the Windows branch in the Git installation locator
after the existing ProgramFiles candidate to also inspect
%ProgramFiles(x86)%\Git\cmd\git.exe and %LOCALAPPDATA%\Programs\Git\cmd\git.exe.
Retrieve each environment variable through _probe.GetEnvironmentVariable and add
valid paths with AddIfExists, preserving the existing PATH probing and
GitExecutablePath override behavior.
In `@src/Beutl.Editor/VersionControl/IProjectVersionControlCoordinator.cs`:
- Around line 9-24: Add XML documentation to RestoreAsync,
RestoreToNewBranchAsync, CreateBranchAsync, and SwitchBranchAsync defining what
true and false represent, including how cancellation, unmet preconditions, and
Git failures are reported. Keep the documented contract consistent with each
member’s actual behavior and distinguish cancellation from other unsuccessful
results where applicable.
In `@src/Beutl.Editor/VersionControl/ProjectConflictMarkerScanner.cs`:
- Around line 25-38: Update the scanner’s recursive enumeration around
EnumerationOptions and Directory.EnumerateFiles so .git directories are pruned
before traversing their contents rather than filtered by IsInsideGitDirectory
afterward. Use a manual recursive walk or equivalent directory-level traversal,
preserve cancellation and inaccessible-directory handling, and continue scanning
only files with extensions from s_projectExtensions.
In `@src/Beutl/Services/VersionControlCoordinator.cs`:
- Around line 501-664: Refactor RunRestoreCycleAsync to use the existing shared
helpers instead of duplicating cycle logic: call EnsureWorktreeOperationAllowed,
GetOpenProject, GetProjectFile, GetTrackedService,
EnsureRepositoryIsNotConflicted, GetHeadAsync, CloseProjectForOperationAsync,
ReopenProjectAsync, HandleCycleFailure, FinishPreservedClose, and
FinishLifecycleOperation where applicable. Preserve restore-specific operations
and outcomes while routing validation, project lifecycle, failure handling, and
cleanup through those helpers.
In `@src/Beutl/ViewModels/MenuBarViewModel.Palette.cs`:
- Around line 33-34: Update EnumeratePaletteCommands in MenuBarViewModel.Palette
so it yields the version-control commands EnableVersionControl and CommitVersion
alongside their FindContextCommand mappings, allowing CommandPaletteService to
expose them. Preserve the existing command ordering and metadata conventions
used by neighboring File menu entries.
In `@src/Beutl/Views/MacWindow.axaml.cs`:
- Around line 137-148: Update the NativeMenu initialization block to locate
viewMenuItem and toolWindowMenu by stable x:Name-based lookup over
NativeMenu.GetMenu(this).Items.OfType<NativeMenuItem>() instead of hardcoded
indices 3 and 4. Add matching names to the XAML menu items and preserve the
existing nested menu assignments for editorTabMenu and toolTabMenu.
In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs`:
- Around line 27-38: Extract the repeated VersionControlConfig setup and
restoration into a small IDisposable scope helper used by the four tests. Have
it capture and restore GitExecutablePath, AutoCommitOnSave, AutoCommitOnClose,
and UseLfsWhenAvailable, apply the requested test values on entry, and restore
all fields on disposal; replace each manual try/finally block with this helper.
In `@tests/Beutl.HeadlessUITests/VersionControlSaveTests.cs`:
- Around line 231-233: Remove the conditional expression in the process
executable resolution and call FindGitOnPath() directly, since
process.StartInfo.FileName is always initialized to "git" in this flow. Keep the
surrounding process setup unchanged.
- Around line 131-143: Remove the duplicate CountSaveSnapshots helper and update
its callers to use CountOccurrences with the "Beutl-Snapshot: save" trailer.
Preserve the existing save-snapshot counting behavior while centralizing
occurrence counting in CountOccurrences.
- Around line 204-282: Extract ProbeGitOrIgnore, FindGitOnPath, RunGitAsync, and
IsolatedGitEnvironment from
tests/Beutl.HeadlessUITests/VersionControlSaveTests.cs:204-282 into one shared
internal test helper class, then update that fixture to use it. In
tests/Beutl.HeadlessUITests/VersionControlTabViewTests.cs:705-764, remove the
local ProbeGitOrIgnore, FindGitOnPath, RunGit, and IsolatedGitEnvironment
implementations and replace their usages with the shared helper, preserving each
fixture’s existing behavior and method conventions.
In `@tests/Beutl.HeadlessUITests/VersionControlTabViewTests.cs`:
- Around line 285-691: Split
Adaptive_layout_supports_onboarding_wide_and_narrow_drill_down into focused
[AvaloniaTest] methods covering setup/onboarding, primary actions and prompts,
notifications and keyboard handling, and wide/narrow layout drill-down behavior.
Extract shared project, window, view, and ViewModel initialization into a
reusable setup helper, and preserve the existing cleanup and assertions within
the relevant tests.
In
`@tests/Beutl.UnitTests/Editor/VersionControl/GitCliVersionControlServiceTests.cs`:
- Around line 820-830: Make the MaxConcurrency update in RunAsync race-free by
atomically retaining the greatest observed concurrency rather than using an
unsynchronized read-modify-write. Preserve the existing concurrency increment
and ensure concurrent Git commands cannot overwrite a higher value with a lower
one.
In `@tests/Beutl.UnitTests/Editor/VersionControl/RealGitTestRepository.cs`:
- Around line 11-18: Update the IsolatedGitEnvironment setup in
RealGitTestRepository to use a platform-portable value for GIT_CONFIG_GLOBAL,
selecting Windows NUL or an empty temporary file instead of the POSIX-only
/dev/null. Also update the matching GIT_CONFIG_GLOBAL assignment in
ProbeGitOrIgnore so all repository fixtures preserve global Git configuration
isolation across platforms.
In `@tests/Beutl.UnitTests/Editor/VersionControl/RemoteOperationsTests.cs`:
- Around line 173-197: Rename the helper method CommitInRepositoryAsync to
CommitAndPushAsync to accurately reflect that it commits and pushes changes, and
update both call sites to use the new name.
- Around line 74-87: Await the post-condition reads from RunGitAsync and
ReadRemoteHeadAsync before the Assert.Multiple block, storing their
trimmed/local and remote head values in variables. Update the assertions to
compare those awaited values while preserving the existing checks and
Assert.Multiple structure.
In `@tests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherStressTests.cs`:
- Around line 62-83: The WaitForStatusCallsToSettleAsync helper should convert
timeout cancellation into a direct assertion failure that reports the observed
status call count; catch the timeout cancellation around the delay and fail with
that context while preserving normal settling behavior. Also update the line 47
assertion message to say “well under 25 status calls,” matching
maximumStatusCalls.
In
`@tests/Beutl.UnitTests/Editor/VersionControl/VersionControlMenuCompletenessTests.cs`:
- Around line 80-94: Update FindRepositoryRoot and the dependent completeness
test to avoid requiring the repository’s source-tree layout at runtime. Prefer
validating the compiled menu definitions or embedding the menu XAML as a test
resource, and remove the DirectoryNotFoundException-based source-path discovery
while preserving the existing command completeness assertions.
In
`@tests/Beutl.UnitTests/Editor/VersionControl/VersionControlPerformanceTests.cs`:
- Around line 9-10: Add an NUnit Category("Performance") attribute to the
VersionControlPerformanceTests fixture so its wall-clock assertions can be
excluded from default CI runs. Keep s_snapshotLimit, s_historyLimit, and the
existing test behavior unchanged.
In
`@tests/Beutl.UnitTests/Editor/VersionControl/VersionControlSnapshotScopeTests.cs`:
- Around line 14-17: Update the scene content written by WriteProjectFileAsync
in VersionControlSnapshotScopeTests so both references include the elements/
directory prefix, matching the on-disk element paths while preserving the
existing element identifiers.
- Around line 55-62: The duplicated CreateService wiring should be centralized
in RealGitTestRepository. Add a protected factory there accepting an optional
runner timeout, then remove the private factories from
tests/Beutl.UnitTests/Editor/VersionControl/VersionControlSnapshotScopeTests.cs:55-62
and
tests/Beutl.UnitTests/Editor/VersionControl/VersionControlPerformanceTests.cs:92-99,
updating callers to use a 30-second timeout; remove the copy from
tests/Beutl.UnitTests/Editor/VersionControl/RemoteOperationsTests.cs:124-131 and
use the default timeout.
In `@tests/Beutl.UnitTests/Editor/VersionControl/VersionControlTabViewTests.cs`:
- Around line 10-13: Remove the duplicate Key.Enter test cases from the
test-case list, keeping the Key.Return cases for both Control and Meta
modifiers. Preserve the existing test coverage with one key alias per modifier.
In `@tests/Beutl.UnitTests/Language/VersionControlStringsTests.cs`:
- Around line 74-88: Update Required_version_control_strings_are_localized to
inspect the culture-specific ResourceSet directly with fallback disabled, so
ja-JP keys cannot resolve from the neutral resource. Handle en-US through
CultureInfo.InvariantCulture because it uses the neutral Strings.resx, while
requiring every other culture’s key to exist and be non-empty in its own
resource set.
In `@tests/Beutl.UnitTests/ProjectSystem/NoMigrationRegressionTests.cs`:
- Around line 82-95: Update the test data in NoMigrationRegressionTests so
json["minAppVersion"] uses a value distinct from Project’s default, while
retaining the existing appVersion assertion and byte-for-byte save comparison.
Ensure the restored project verifies the non-default minAppVersion also survives
the plain save.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac1399e5d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review resolution update for
Left open for human design decisions:
Validation:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/specs/005-project-git-versioning/tasks.md (3)
149-149: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the dependency graph for T027.
Line 149 states that T027 runs after T023 because it changes the Version Control tab. Line 157 lists T027 in parallel with T028 without naming T023 as a prerequisite.
Update the parallelization section, or split T027 into tab-independent and tab-dependent tasks.
Also applies to: 157-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/005-project-git-versioning/tasks.md` at line 149, Align the dependency graph for T027 across the dependency and parallelization sections: ensure the parallelization entry listing T027 with T028 also declares T023 as a prerequisite, or split T027 so only its tab-dependent portion waits for T023 while tab-independent work remains parallel.
141-141: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftComplete the pending manual verification matrix.
T040 is still unchecked and covers remote push/pull via credential helpers, SSH/agent auth, LFS round-trip, macOS git discovery, and notarized-bundle spawn. These release-critical paths are outside the local build/test results recorded in T039. Run the matrix, record the results in the PR, and include the R-10.4 Windows newline release-notes callout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/005-project-git-versioning/tasks.md` at line 141, Complete T040 by running every quickstart.md manual verification matrix row, including credential-helper and SSH/agent remote push/pull, LFS round-trip, macOS Git discovery, and notarized-bundle spawning. Record each result in the PR description and add the R-10.4 release-notes callout for the one-time Windows newline diff, then mark T040 complete.
51-58: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle missing identity before creating the initial snapshot.
T012 has
InitializeAsynccreate theBeutl-Snapshot: initcommit directly, and T013/SnapshotKindInitis non-Manual, soInitializeAsynccurrently returnsSkippedNoIdentitywhenuser.name/user.emailare unset. No repository-local identity can exist before the repo is initialized, so add an explicit initialization identity flow or pending state and a no-global-identity test for the baseline commit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/005-project-git-versioning/tasks.md` around lines 51 - 58, Update InitializeAsync and its initial Beutl-Snapshot: init commit flow so a repository without global identity can still establish or request a repository-local identity before creating the baseline commit, rather than returning SkippedNoIdentity. Preserve repo-local identity semantics, and add coverage verifying the initial commit succeeds when no global Git identity is configured.src/Beutl/Services/VersionControlCoordinator.cs (3)
431-521: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
RunPullCycleAsynccan swallow a critical recovery failure on cancellation.Lines 491-494 re-throw
OperationCanceledExceptionbefore checkingrecoveryFailureat lines 496-507. If pull is cancelled after the project was closed for the operation andTryRestoreOriginalStateAsyncthen also fails, the method re-throws immediately and never logs or notifies about the failed recovery. The project can be left in a bad state without the user being told.
HandleCycleFailure(used byRunBranchCycleAsync) and the catch block inRunRestoreCycleAsyncboth checkrecoveryFailurebefore considering the cancellation re-throw. Reorder the checks inRunPullCycleAsyncto match.🐛 Proposed fix: check `recoveryFailure` before re-throwing cancellation
catch (Exception ex) { Exception? recoveryFailure = projectClosed ? await TryRestoreOriginalStateAsync( service, originalHead.Sha, originalBranch, branchMayHaveChanged: false, projectFile) : null; - if (ex is OperationCanceledException && cancellationToken.IsCancellationRequested) - { - throw; - } - if (recoveryFailure is not null) { HandleCycleFailure( ex, recoveryFailure, "pull", cancellationToken); return new RemoteOpResult.Failed(string.Format( Strings.VersionControl_RecoveryFailed, GetErrorText(ex), GetErrorText(recoveryFailure))); } + if (ex is OperationCanceledException && cancellationToken.IsCancellationRequested) + { + throw; + } + _logger.LogError(ex, "Failed to pull project versions."); return new RemoteOpResult.Failed(GetErrorText(ex)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Services/VersionControlCoordinator.cs` around lines 431 - 521, Reorder the exception handling in RunPullCycleAsync so recoveryFailure is processed before rethrowing a cancellation exception. When restoration fails, invoke HandleCycleFailure and return the recovery-failed result even if cancellation was requested; only rethrow OperationCanceledException when no recovery failure occurred, matching RunBranchCycleAsync and RunRestoreCycleAsync.
299-332: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnsynchronized access to
_activationCancellationcan crashDispose().
Dispose()calls_activationCancellation?.Cancel(); _activationCancellation?.Dispose();at lines 311-312 without holding_stateGate.CancelActivation()at lines 1196-1203 mutates the same field (.Cancel(),.Dispose(), then sets it tonull), and it runs fromOnProjectChanged, which only checksif (_disposed) return;at its own top without acquiring_stateGate.If
Dispose()and a project-change notification run concurrently, one thread can call.Cancel()on aCancellationTokenSourcethe other thread has already disposed.CancellationTokenSource.Cancel()throwsObjectDisposedExceptionwhen the source was already disposed, so this is a real crash path, not just a style concern.Every other piece of mutable coordinator state (
_isGitAvailable,_isTracked,_currentService) is already guarded by_stateGateplus a_disposedcheck in this same file. Apply the same pattern to_activationCancellation.🔒 Proposed fix: guard `_activationCancellation` with `_stateGate`
public void Dispose() { + CancellationTokenSource? activationCancellation; lock (_stateGate) { if (_disposed) { return; } _disposed = true; + activationCancellation = _activationCancellation; + _activationCancellation = null; } - _activationCancellation?.Cancel(); - _activationCancellation?.Dispose(); + activationCancellation?.Cancel(); + activationCancellation?.Dispose(); _config.ConfigurationChanged -= OnVersionControlConfigChanged;private void CancelActivation() { - _activationRevision++; - _activationCancellation?.Cancel(); - _activationCancellation?.Dispose(); - _activationCancellation = null; - _activationTask = Task.CompletedTask; + CancellationTokenSource? activationCancellation; + lock (_stateGate) + { + _activationRevision++; + activationCancellation = _activationCancellation; + _activationCancellation = null; + _activationTask = Task.CompletedTask; + } + + activationCancellation?.Cancel(); + activationCancellation?.Dispose(); }Also applies to: 1196-1203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Services/VersionControlCoordinator.cs` around lines 299 - 332, Guard all access and mutation of _activationCancellation with _stateGate, including Dispose() and CancelActivation(). Coordinate cancellation, disposal, and setting the field to null under the same lock, while preserving the existing _disposed checks and activation behavior to prevent concurrent use of a disposed CancellationTokenSource.
523-723: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
RunRestoreCycleAsyncduplicates the shared lifecycle helpers instead of reusing them.This method reimplements, inline, nearly every helper extracted for
RunBranchCycleAsync/RunPullCycleAsync: the export-in-progress check (compare lines 537-543 withEnsureWorktreeOperationAllowed), project/service lookup (545-557 vsGetOpenProject/GetProjectFile/GetTrackedService), the conflict check (560-566 vsEnsureRepositoryIsNotConflicted), the HEAD lookup (582-588 vsGetHeadAsync), the close-and-verify step (594-600 vsCloseProjectForOperationAsync), the reopen-and-verify step (616-617 vsReopenProjectAsync), and the twofinallyblocks (666-672 vsFinishPreservedClose, 675-685 vsFinishLifecycleOperation).This duplication already caused real drift:
RunPullCycleAsyncchecks cancellation beforerecoveryFailurein its own hand-rolled catch block (flagged separately), whileRunRestoreCycleAsyncand the sharedHandleCycleFailureget the order right. Reusing the shared helpers here removes that risk of future divergence.♻️ Proposed fix: reuse the shared helpers
- if (_editorService.IsExportRunning) - { - NotificationService.ShowWarning( - Strings.VersionControl, - Strings.VersionControl_ExportInProgress); - return false; - } - - Project project = _projectService.CurrentProject.Value - ?? throw new InvalidOperationException("No project is open."); - string projectFile = project.Uri?.LocalPath - ?? throw new InvalidOperationException( - "The project has no file path."); - IProjectVersionControlService service = _currentService - ?? throw new InvalidOperationException( - "Version control is not available."); - if (service.Repository is null) - { - throw new InvalidOperationException( - "The open project is not tracked with Git."); - } - - WorkspaceStatus status = await service.GetStatusAsync(cancellationToken); - if (status.HasConflicts) - { - NotificationService.ShowWarning( - Strings.VersionControl, - Strings.VersionControl_ConflictGuidance); - return false; - } + if (!EnsureWorktreeOperationAllowed()) + { + return false; + } + + Project project = GetOpenProject(); + string projectFile = GetProjectFile(project); + IProjectVersionControlService service = GetTrackedService(); + WorkspaceStatus status = await service.GetStatusAsync(cancellationToken); + if (!EnsureRepositoryIsNotConflicted(status)) + { + return false; + }Apply the same substitution to the HEAD lookup (use
GetHeadAsync), the close/reopen steps (useCloseProjectForOperationAsync/ReopenProjectAsync), and bothfinallyblocks (useFinishPreservedClose/FinishLifecycleOperation).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Services/VersionControlCoordinator.cs` around lines 523 - 723, Refactor RunRestoreCycleAsync to reuse the existing lifecycle helpers instead of duplicating their logic: call EnsureWorktreeOperationAllowed, GetOpenProject, GetProjectFile, GetTrackedService, EnsureRepositoryIsNotConflicted, and GetHeadAsync; replace inline close/reopen verification with CloseProjectForOperationAsync and ReopenProjectAsync; and replace both finally-block implementations with FinishPreservedClose and FinishLifecycleOperation. Preserve the current restore, recovery, and error-handling behavior while using the shared helpers consistently.
🧹 Nitpick comments (1)
tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs (1)
571-581: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
WaitUntilAsyncpolling helper.This
WaitUntilAsyncimplementation is identical to the one already defined intests/Beutl.HeadlessUITests/VersionControlTabViewTests.csandtests/Beutl.HeadlessUITests/CreateNewProjectDialogTests.cs. Move this helper into a shared test utility (for example alongsideHeadlessTestHelpersorTestReset) and have all three test classes call the shared version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs` around lines 571 - 581, Extract the duplicated WaitUntilAsync helper from VersionControlRestoreTests, VersionControlTabViewTests, and CreateNewProjectDialogTests into a shared test utility alongside HeadlessTestHelpers or TestReset. Update all three test classes to call the shared implementation and remove their local copies, preserving the existing polling, timeout, settling, delay, and assertion behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/specs/005-project-git-versioning/tasks.md`:
- Around line 51-52: Make nested-repository discovery and user-consent
validation a prerequisite of InitializeAsync before any git init execution,
rather than relying only on the later T026 flow. Preserve the existing
initialization behavior after the guard passes, and add coverage verifying
InitializeAsync does not create a nested .git directory when the project is
inside an enclosing repository without consent.
In `@tests/Beutl.HeadlessUITests/CreateNewProjectDialogTests.cs`:
- Around line 70-98: The test
Enable_version_control_command_is_gated_by_the_open_project_state_and_mapped_as_a_context_command
must not assume Git auto-discovery succeeds. Configure
GlobalConfiguration.Instance.VersionControlConfig.GitExecutablePath using the
existing ProbeGitOrIgnore() pattern before asserting the opened-project command
is enabled, or skip that enabled assertion when Git is unavailable while
preserving the initial disabled-state and context-command checks.
---
Outside diff comments:
In `@docs/specs/005-project-git-versioning/tasks.md`:
- Line 149: Align the dependency graph for T027 across the dependency and
parallelization sections: ensure the parallelization entry listing T027 with
T028 also declares T023 as a prerequisite, or split T027 so only its
tab-dependent portion waits for T023 while tab-independent work remains
parallel.
- Line 141: Complete T040 by running every quickstart.md manual verification
matrix row, including credential-helper and SSH/agent remote push/pull, LFS
round-trip, macOS Git discovery, and notarized-bundle spawning. Record each
result in the PR description and add the R-10.4 release-notes callout for the
one-time Windows newline diff, then mark T040 complete.
- Around line 51-58: Update InitializeAsync and its initial Beutl-Snapshot: init
commit flow so a repository without global identity can still establish or
request a repository-local identity before creating the baseline commit, rather
than returning SkippedNoIdentity. Preserve repo-local identity semantics, and
add coverage verifying the initial commit succeeds when no global Git identity
is configured.
In `@src/Beutl/Services/VersionControlCoordinator.cs`:
- Around line 431-521: Reorder the exception handling in RunPullCycleAsync so
recoveryFailure is processed before rethrowing a cancellation exception. When
restoration fails, invoke HandleCycleFailure and return the recovery-failed
result even if cancellation was requested; only rethrow
OperationCanceledException when no recovery failure occurred, matching
RunBranchCycleAsync and RunRestoreCycleAsync.
- Around line 299-332: Guard all access and mutation of _activationCancellation
with _stateGate, including Dispose() and CancelActivation(). Coordinate
cancellation, disposal, and setting the field to null under the same lock, while
preserving the existing _disposed checks and activation behavior to prevent
concurrent use of a disposed CancellationTokenSource.
- Around line 523-723: Refactor RunRestoreCycleAsync to reuse the existing
lifecycle helpers instead of duplicating their logic: call
EnsureWorktreeOperationAllowed, GetOpenProject, GetProjectFile,
GetTrackedService, EnsureRepositoryIsNotConflicted, and GetHeadAsync; replace
inline close/reopen verification with CloseProjectForOperationAsync and
ReopenProjectAsync; and replace both finally-block implementations with
FinishPreservedClose and FinishLifecycleOperation. Preserve the current restore,
recovery, and error-handling behavior while using the shared helpers
consistently.
---
Nitpick comments:
In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs`:
- Around line 571-581: Extract the duplicated WaitUntilAsync helper from
VersionControlRestoreTests, VersionControlTabViewTests, and
CreateNewProjectDialogTests into a shared test utility alongside
HeadlessTestHelpers or TestReset. Update all three test classes to call the
shared implementation and remove their local copies, preserving the existing
polling, timeout, settling, delay, and assertion behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a98ff0e-b452-400d-ac9f-55f0708ae494
📒 Files selected for processing (42)
README.mddocs/specs/005-project-git-versioning/contracts/coordinator-lifecycle.mddocs/specs/005-project-git-versioning/contracts/git-cli-invocation.mddocs/specs/005-project-git-versioning/contracts/version-control-service.mddocs/specs/005-project-git-versioning/data-model.mddocs/specs/005-project-git-versioning/quickstart.mddocs/specs/005-project-git-versioning/research.mddocs/specs/005-project-git-versioning/spec.mddocs/specs/005-project-git-versioning/tasks.mdsrc/Beutl.Editor.Components/VersionControl/ViewModels/TitleBarBranchViewModel.cssrc/Beutl.Editor.Components/VersionControlTab/Views/VersionControlChangesView.axaml.cssrc/Beutl.Editor.Components/VersionControlTab/Views/VersionControlHistoryView.axaml.cssrc/Beutl.Editor/VersionControl/GitCliRunner.cssrc/Beutl.Editor/VersionControl/GitCliVersionControlService.cssrc/Beutl.Editor/VersionControl/GitInstallationLocator.cssrc/Beutl.Editor/VersionControl/IProjectVersionControlInitializer.cssrc/Beutl.Editor/VersionControl/RepositoryWatcher.cssrc/Beutl.Editor/VersionControl/VersionControlModels.cssrc/Beutl.Language/Strings.ja.resxsrc/Beutl.Language/Strings.resxsrc/Beutl/Services/VersionControlCoordinator.cssrc/Beutl/ViewModels/Dialogs/CreateNewProjectViewModel.cssrc/Beutl/ViewModels/EditContext/ElementAdderImpl.cssrc/Beutl/ViewModels/MenuBarViewModel.Files.cssrc/Beutl/Views/TitleBarBranchView.axaml.cstests/Beutl.HeadlessUITests/CreateNewProjectDialogTests.cstests/Beutl.HeadlessUITests/EditorWorkflowTests.cstests/Beutl.HeadlessUITests/VersionControlConflictTests.cstests/Beutl.HeadlessUITests/VersionControlRestoreTests.cstests/Beutl.HeadlessUITests/VersionControlSaveTests.cstests/Beutl.HeadlessUITests/VersionControlTabViewTests.cstests/Beutl.UnitTests/Editor/VersionControl/GitCliRunnerTests.cstests/Beutl.UnitTests/Editor/VersionControl/GitCliVersionControlServiceTests.cstests/Beutl.UnitTests/Editor/VersionControl/GitInstallationLocatorTests.cstests/Beutl.UnitTests/Editor/VersionControl/NestedRepositoryTests.cstests/Beutl.UnitTests/Editor/VersionControl/RealGitTestRepository.cstests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherStressTests.cstests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherTests.cstests/Beutl.UnitTests/Editor/VersionControl/TitleBarBranchViewModelTests.cstests/Beutl.UnitTests/Editor/VersionControl/VersionControlModelsTests.cstests/Beutl.UnitTests/Editor/VersionControl/VersionControlTabViewModelTests.cstests/Beutl.UnitTests/Editor/VersionControl/VersionControlTabViewTests.cs
🚧 Files skipped from review as they are similar to previous changes (19)
- src/Beutl.Language/Strings.resx
- src/Beutl.Editor.Components/VersionControlTab/Views/VersionControlHistoryView.axaml.cs
- src/Beutl.Language/Strings.ja.resx
- docs/specs/005-project-git-versioning/quickstart.md
- docs/specs/005-project-git-versioning/contracts/version-control-service.md
- tests/Beutl.HeadlessUITests/VersionControlConflictTests.cs
- src/Beutl/ViewModels/Dialogs/CreateNewProjectViewModel.cs
- src/Beutl.Editor/VersionControl/GitInstallationLocator.cs
- docs/specs/005-project-git-versioning/contracts/coordinator-lifecycle.md
- docs/specs/005-project-git-versioning/data-model.md
- src/Beutl/ViewModels/MenuBarViewModel.Files.cs
- docs/specs/005-project-git-versioning/contracts/git-cli-invocation.md
- tests/Beutl.UnitTests/Editor/VersionControl/TitleBarBranchViewModelTests.cs
- src/Beutl.Editor.Components/VersionControl/ViewModels/TitleBarBranchViewModel.cs
- tests/Beutl.UnitTests/Editor/VersionControl/RealGitTestRepository.cs
- tests/Beutl.UnitTests/Editor/VersionControl/VersionControlTabViewModelTests.cs
- src/Beutl.Editor/VersionControl/GitCliRunner.cs
- src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs
- docs/specs/005-project-git-versioning/research.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d27c151a2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Beutl/Views/MacWindow.axaml.cs (1)
142-145: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the root menu indices before reading extension menus.
MacWindow.axamlstill declares the root menu order as File, CreateNew submenu, Open submenu, RecentFiles, RecentProjects, Project, Edit, View, Editors submenu, Tools submenu, Tools, Scene, Element, Help.rootMenu.Items[3]androotMenu.Items[4]still point toRecentFilesandRecentProjects, soInitExtMenuItemsselects the wrong native menus on macOS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Views/MacWindow.axaml.cs` around lines 142 - 145, Update the rootMenu index references in InitExtMenuItems so viewMenuItem resolves the View entry and toolWindowMenu resolves the Tools submenu according to the declared MacWindow.axaml order; keep editorTabMenu and toolTabMenu based on the selected View menu’s entries.docs/specs/005-project-git-versioning/tasks.md (2)
58-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope the E2E assertion to the save snapshot.
Initialization creates an
initcommit before the explicit save. State that the test expects exactly one additional commit with theBeutl-Snapshot: savetrailer, and no additional commit for a clean save. This prevents the test from counting the initialization or close snapshot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/005-project-git-versioning/tasks.md` at line 58, Update task T019 to scope the E2E assertion to the explicit save snapshot: expect exactly one additional commit carrying the “Beutl-Snapshot: save” trailer after initialization, and no additional commit when the save is clean; exclude the initialization and close snapshots from the count.
141-141: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftComplete or explicitly defer the release-critical verification matrix.
T040 remains unchecked while the plan states that all six stories are functional. The matrix covers credentials, network behavior, LFS, macOS discovery, and notarization. Complete it before release, or document the unverified rows and residual risk in the PR description.
As per coding guidelines, production changes require an NUnit test or documented manual verification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/005-project-git-versioning/tasks.md` at line 141, Update task T040 by completing the quickstart.md manual verification matrix before release and recording results for credentials, network behavior, LFS, macOS discovery, notarization, and the Windows newline-diff release-note callout in the PR description; if any rows remain unverified, explicitly document them and the associated residual risk instead.Source: Coding guidelines
src/Beutl.Editor/VersionControl/GitCliRunner.cs (1)
221-237: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAlso observe
OperationCanceledExceptionfrom the standard-input task.
stdinTaskwrites withlinkedCts.Token. When the local timeout fires during a write,WriteStandardInputAsyncfaults withOperationCanceledException.ObserveReaderAfterProcessExitAsynconly filtersIOExceptionandObjectDisposedException, so awaiting the faultedstdinTaskrethrows that exception from inside the catch block. The timeout translation on Line 230 andcancellationToken.ThrowIfCancellationRequested()on Line 235 are then skipped, and a Git timeout surfaces as a cancellation to the caller.This path is reachable for commands that send stdin, for example
check-ignore --stdin -z.🐛 Proposed fix
private static async Task ObserveReaderAfterProcessExitAsync<T>(Task<T> readerTask) { try { await readerTask.ConfigureAwait(false); } - catch (Exception ex) when (ex is IOException or ObjectDisposedException) + catch (Exception ex) when (ex is IOException + or ObjectDisposedException + or OperationCanceledException) { } } private static async Task ObserveReaderAfterProcessExitAsync(Task readerTask) { try { await readerTask.ConfigureAwait(false); } - catch (Exception ex) when (ex is IOException or ObjectDisposedException) + catch (Exception ex) when (ex is IOException + or ObjectDisposedException + or OperationCanceledException) { } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor/VersionControl/GitCliRunner.cs` around lines 221 - 237, Update ObserveReaderAfterProcessExitAsync handling in the OperationCanceledException catch path so OperationCanceledException from stdinTask cancellation is observed without escaping. Preserve propagation of the outer cancellation and timeout translation, ensuring timed-out stdin writes still reach the timeout check and cancellationToken.ThrowIfCancellationRequested().
🧹 Nitpick comments (7)
tests/Beutl.UnitTests/Editor/VersionControl/RemoteOperationsTests.cs (1)
1648-1652: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
GitCommandOptions.Networkfor thepushinCommitInRepositoryAsync.The other
pushcall sites in this file passGitCommandOptions.Network(Lines 141-145 and Lines 226-230). This helper passesGitCommandOptions.Localfor the same command. The Git CLI invocation contract separates local and network options, including timeout and SSH environment handling. Align this helper so peer pushes exercise the same runner path.♻️ Proposed fix
await runner.RunAsync( repository, ["push"], - GitCommandOptions.Local, + GitCommandOptions.Network, CancellationToken.None);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.UnitTests/Editor/VersionControl/RemoteOperationsTests.cs` around lines 1648 - 1652, Update the push invocation in CommitInRepositoryAsync to pass GitCommandOptions.Network instead of GitCommandOptions.Local, matching the other push call sites and ensuring it uses the network runner path.src/Beutl.Editor/VersionControl/VersionControlModels.cs (1)
342-347: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSanitize
stderronce.
RedactCredentialsruns twice on the same input: once for the base message and once for theStderrproperty. Compute the sanitized value once so the message and the property cannot diverge.♻️ Proposed refactor
- public GitOperationException(int exitCode, string stderr) - : base(CreateMessage(exitCode, GitDiagnosticSanitizer.RedactCredentials(stderr))) - { - ExitCode = exitCode; - Stderr = GitDiagnosticSanitizer.RedactCredentials(stderr); - } + public GitOperationException(int exitCode, string stderr) + : this(exitCode, GitDiagnosticSanitizer.RedactCredentials(stderr), sanitized: true) + { + } + + private GitOperationException(int exitCode, string sanitizedStderr, bool sanitized) + : base(CreateMessage(exitCode, sanitizedStderr)) + { + ExitCode = exitCode; + Stderr = sanitizedStderr; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor/VersionControl/VersionControlModels.cs` around lines 342 - 347, Update the GitOperationException constructor to sanitize stderr only once before constructing the exception message, then reuse that sanitized value for both CreateMessage and the Stderr property. Preserve the existing message and property behavior while eliminating the duplicate GitDiagnosticSanitizer.RedactCredentials call.tests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherStressTests.cs (1)
198-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the probe policy from
optionsinstead of hardcodingLocal.The method now receives
GitCommandOptions optionsand forwards it at line 218. TheGIT_OPTIONAL_LOCKSprobe at line 206 still builds its start info with a hardcodedGitExecutionPolicy.Local, so the assertion inspects a synthesized start info rather than the configuration used for the forwarded call.Today every
statuscall usesGitCommandOptions.Local, so the check passes for the right reason. If astatuscall later runs under a different policy or with environment overrides, the assertion would still pass while the real process omittedGIT_OPTIONAL_LOCKS=0. Mapoptionsto its execution policy for the probe so the guard tracks the actual call.This test is the guard for the repository rule that Git commands set
GIT_OPTIONAL_LOCKS=0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherStressTests.cs` around lines 198 - 221, Update the GIT_OPTIONAL_LOCKS probe in the status branch of the command wrapper to derive the execution policy from the received GitCommandOptions options instead of hardcoding GitExecutionPolicy.Local. Build the probe start info with that mapped policy so it reflects the same configuration forwarded to inner.RunAsync.Source: Coding guidelines
src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs (2)
3954-3982: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead the hygiene file once.
EnsureLinesAsynccallsFile.Existstwice and reads the same file twice, throughReadAllTextAsyncand thenReadAllLinesAsync. Read the text once and split it, so the comparison and the line list come from one snapshot.♻️ Proposed refactor
- string? existingContents = File.Exists(path) - ? await File.ReadAllTextAsync(path, cancellationToken).ConfigureAwait(false) - : null; - var lines = File.Exists(path) - ? (await File.ReadAllLinesAsync(path, cancellationToken).ConfigureAwait(false)).ToList() - : []; + string? existingContents = File.Exists(path) + ? await File.ReadAllTextAsync(path, cancellationToken).ConfigureAwait(false) + : null; + List<string> lines = existingContents is null + ? [] + : [.. existingContents + .Replace("\r\n", "\n", StringComparison.Ordinal) + .Split('\n', StringSplitOptions.RemoveEmptyEntries)];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs` around lines 3954 - 3982, Update EnsureLinesAsync to read the hygiene file contents only once, using a single existence check and ReadAllTextAsync result to derive the existing text and mutable line list. Preserve the current missing-file behavior, required-line insertion, exact contents comparison, and UTF-8 write behavior while eliminating the separate ReadAllLinesAsync call.
2119-2435: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the transition recovery logic into named helpers.
ApplyTreeTransitionAsyncspans roughly 580 lines, and thiscatchblock alone holds five nested decision levels across index ownership, worktree ownership, temporary-head alignment, index restore, and final verification. The control flow is correct as written, but the nesting makes each outcome hard to audit against the four documentedPullTransitionStatevalues.Extract the recovery block into helpers such as
EvaluateOwnershipAsync,AlignTemporaryHeadAsync, andVerifyRestoredStateAsync. Keep the behavior identical so the existing regression tests still pin the outcomes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs` around lines 2119 - 2435, Refactor the recovery logic in ApplyTreeTransitionAsync into focused named helpers, including ownership evaluation, temporary-head alignment, index/worktree restoration, and final-state verification. Preserve the existing decision order, exception aggregation, Git operations, and TreeTransitionOutcome results exactly; use helpers such as EvaluateOwnershipAsync, AlignTemporaryHeadAsync, and VerifyRestoredStateAsync to reduce nesting without changing behavior.src/Beutl.Editor/VersionControl/RepositoryWatcher.cs (1)
235-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
_watcherslocking invariant forAddGitMetadataWatcher.
AddGitMetadataWatchermutates_watchersunder two different calling conventions.RefreshGitRefsWatchercalls it while holding_sync.StartandAddGitMetadataWatcherscall it without the lock, and they are safe only because they run during construction. A later change that callsAddGitMetadataWatchersafter construction would create an unguarded mutation and could leak a watcher pastDispose.Add a short comment that states the invariant, or move the
_watchers.Addcall into a helper that always takes_sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Editor/VersionControl/RepositoryWatcher.cs` around lines 235 - 263, Document the locking invariant in AddGitMetadataWatcher near the _watchers.Add mutation: RefreshGitRefsWatcher invokes it while holding _sync, while Start and AddGitMetadataWatchers call it only during construction without the lock. State that any future post-construction caller must hold _sync, or route the mutation through a helper that consistently acquires the lock.src/Beutl/Services/VersionControlCoordinator.cs (1)
1069-1081: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
FinishLifecycleOperationin the restore cycle.The
finallyblock duplicatesFinishLifecycleOperation(bool)(Lines 1331-1342). The branch and pull cycles already call the helper. Use the helper here so the lifecycle-cleanup rule stays in one place.♻️ Proposed refactor
finally { - if (gateEntered) - { - _lifecycleGate.Release(); - } - - if (Interlocked.Decrement(ref _lifecycleUsers) == 0 && _disposed) - { - ClearProjectState(); - } + FinishLifecycleOperation(gateEntered); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl/Services/VersionControlCoordinator.cs` around lines 1069 - 1081, Replace the duplicated lifecycle cleanup in the restore cycle’s finally block with a call to FinishLifecycleOperation(bool), passing the existing gateEntered state. Preserve the current release and disposed-user cleanup behavior through the helper, matching the branch and pull cycle usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/specs/005-project-git-versioning/research.md`:
- Line 107: Correct the call-site count in the “Id-based element file names”
item from five to six, matching the six listed RandomFileNameGenerator usages
and the T006 scope; keep all referenced files and symbols unchanged.
- Line 30: Update the R-2 environment list in the project Git versioning
research specification to include GIT_LITERAL_PATHSPECS=1 for ordinary project
path arguments, while documenting that the NUL-delimited git check-ignore
--stdin -z probe remains the sole exception using 0.
In `@src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs`:
- Around line 4183-4196: Update CaptureRecoverableLock to read _runner once into
a local variable before the null check, then validate and dereference that local
when calling GetRecoverableRepositoryLock. Preserve the existing early returns
for missing repository or runner while preventing the field from being reread
after validation.
In `@tests/Beutl.HeadlessUITests/ShutdownPipelineTests.cs`:
- Around line 124-137: Add await TestReset.ResetShellAsync() as the first
statement in
Shutdown_waits_for_version_control_transition_then_performs_final_close, before
SetOpenProject or any other state mutation, so global shell singletons are reset
for this test.
In `@tests/Beutl.HeadlessUITests/VersionControlTabViewTests.cs`:
- Around line 800-802: In the commit message setup within
VersionControlTabViewTests, add the established UI-settle call after assigning
“first line” to commitMessageTextBox’s view-model value and before reading
commitMessageTextBox.Text for CaretIndex. Keep the existing focus and caret
placement behavior unchanged.
---
Outside diff comments:
In `@docs/specs/005-project-git-versioning/tasks.md`:
- Line 58: Update task T019 to scope the E2E assertion to the explicit save
snapshot: expect exactly one additional commit carrying the “Beutl-Snapshot:
save” trailer after initialization, and no additional commit when the save is
clean; exclude the initialization and close snapshots from the count.
- Line 141: Update task T040 by completing the quickstart.md manual verification
matrix before release and recording results for credentials, network behavior,
LFS, macOS discovery, notarization, and the Windows newline-diff release-note
callout in the PR description; if any rows remain unverified, explicitly
document them and the associated residual risk instead.
In `@src/Beutl.Editor/VersionControl/GitCliRunner.cs`:
- Around line 221-237: Update ObserveReaderAfterProcessExitAsync handling in the
OperationCanceledException catch path so OperationCanceledException from
stdinTask cancellation is observed without escaping. Preserve propagation of the
outer cancellation and timeout translation, ensuring timed-out stdin writes
still reach the timeout check and
cancellationToken.ThrowIfCancellationRequested().
In `@src/Beutl/Views/MacWindow.axaml.cs`:
- Around line 142-145: Update the rootMenu index references in InitExtMenuItems
so viewMenuItem resolves the View entry and toolWindowMenu resolves the Tools
submenu according to the declared MacWindow.axaml order; keep editorTabMenu and
toolTabMenu based on the selected View menu’s entries.
---
Nitpick comments:
In `@src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs`:
- Around line 3954-3982: Update EnsureLinesAsync to read the hygiene file
contents only once, using a single existence check and ReadAllTextAsync result
to derive the existing text and mutable line list. Preserve the current
missing-file behavior, required-line insertion, exact contents comparison, and
UTF-8 write behavior while eliminating the separate ReadAllLinesAsync call.
- Around line 2119-2435: Refactor the recovery logic in ApplyTreeTransitionAsync
into focused named helpers, including ownership evaluation, temporary-head
alignment, index/worktree restoration, and final-state verification. Preserve
the existing decision order, exception aggregation, Git operations, and
TreeTransitionOutcome results exactly; use helpers such as
EvaluateOwnershipAsync, AlignTemporaryHeadAsync, and VerifyRestoredStateAsync to
reduce nesting without changing behavior.
In `@src/Beutl.Editor/VersionControl/RepositoryWatcher.cs`:
- Around line 235-263: Document the locking invariant in AddGitMetadataWatcher
near the _watchers.Add mutation: RefreshGitRefsWatcher invokes it while holding
_sync, while Start and AddGitMetadataWatchers call it only during construction
without the lock. State that any future post-construction caller must hold
_sync, or route the mutation through a helper that consistently acquires the
lock.
In `@src/Beutl.Editor/VersionControl/VersionControlModels.cs`:
- Around line 342-347: Update the GitOperationException constructor to sanitize
stderr only once before constructing the exception message, then reuse that
sanitized value for both CreateMessage and the Stderr property. Preserve the
existing message and property behavior while eliminating the duplicate
GitDiagnosticSanitizer.RedactCredentials call.
In `@src/Beutl/Services/VersionControlCoordinator.cs`:
- Around line 1069-1081: Replace the duplicated lifecycle cleanup in the restore
cycle’s finally block with a call to FinishLifecycleOperation(bool), passing the
existing gateEntered state. Preserve the current release and disposed-user
cleanup behavior through the helper, matching the branch and pull cycle usage.
In `@tests/Beutl.UnitTests/Editor/VersionControl/RemoteOperationsTests.cs`:
- Around line 1648-1652: Update the push invocation in CommitInRepositoryAsync
to pass GitCommandOptions.Network instead of GitCommandOptions.Local, matching
the other push call sites and ensuring it uses the network runner path.
In `@tests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherStressTests.cs`:
- Around line 198-221: Update the GIT_OPTIONAL_LOCKS probe in the status branch
of the command wrapper to derive the execution policy from the received
GitCommandOptions options instead of hardcoding GitExecutionPolicy.Local. Build
the probe start info with that mapped policy so it reflects the same
configuration forwarded to inner.RunAsync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c8f3c2b0-1bea-4608-9cbc-0489b0049395
📒 Files selected for processing (65)
CLAUDE.mddocs/specs/005-project-git-versioning/contracts/coordinator-lifecycle.mddocs/specs/005-project-git-versioning/contracts/git-cli-invocation.mddocs/specs/005-project-git-versioning/contracts/version-control-service.mddocs/specs/005-project-git-versioning/data-model.mddocs/specs/005-project-git-versioning/plan.mddocs/specs/005-project-git-versioning/quickstart.mddocs/specs/005-project-git-versioning/research.mddocs/specs/005-project-git-versioning/spec.mddocs/specs/005-project-git-versioning/tasks.mdsrc/Beutl.Core/Project.cssrc/Beutl.Core/ProjectItem.cssrc/Beutl.Editor.Components/VersionControl/Views/VersionControlPickerFlyout.cssrc/Beutl.Editor.Components/VersionControlTab/ViewModels/VersionControlTabViewModel.cssrc/Beutl.Editor/Beutl.Editor.csprojsrc/Beutl.Editor/VersionControl/GitCliRunner.cssrc/Beutl.Editor/VersionControl/GitCliVersionControlService.cssrc/Beutl.Editor/VersionControl/IProjectVersionControlCoordinator.cssrc/Beutl.Editor/VersionControl/IProjectVersionControlInitializer.cssrc/Beutl.Editor/VersionControl/IProjectVersionControlService.cssrc/Beutl.Editor/VersionControl/ProjectConflictMarkerScanner.cssrc/Beutl.Editor/VersionControl/RepositoryWatcher.cssrc/Beutl.Editor/VersionControl/VersionControlModels.cssrc/Beutl.Language/SettingsStrings.ja.resxsrc/Beutl.Language/SettingsStrings.resxsrc/Beutl.Language/Strings.ja.resxsrc/Beutl.Language/Strings.resxsrc/Beutl.ProjectSystem/ProjectSystem/Element.cssrc/Beutl.ProjectSystem/ProjectSystem/ElementMigration.cssrc/Beutl.ProjectSystem/ProjectSystem/Scene.cssrc/Beutl/Pages/SettingsPages/EditorSettingsPage.axamlsrc/Beutl/Services/EditorService.cssrc/Beutl/Services/OutputService.cssrc/Beutl/Services/ProjectService.cssrc/Beutl/Services/VersionControlCoordinator.cssrc/Beutl/ViewModels/Dialogs/CreateNewProjectViewModel.cssrc/Beutl/ViewModels/Dialogs/GitIdentityDialogViewModel.cssrc/Beutl/ViewModels/MainViewModel.cssrc/Beutl/ViewModels/SettingsPages/EditorSettingsPageViewModel.cssrc/Beutl/ViewModels/Tools/OutputTabViewModel.cssrc/Beutl/Views/MacWindow.axaml.cssrc/Beutl/Views/MainView.axaml.InitializeMenuBar.cssrc/Beutl/Views/MainWindow.axaml.cssrc/Beutl/Views/WindowShutdownCoordinator.cstests/Beutl.HeadlessUITests/CreateNewProjectDialogTests.cstests/Beutl.HeadlessUITests/EditorSettingsPageViewModelTests.cstests/Beutl.HeadlessUITests/ExportTests.cstests/Beutl.HeadlessUITests/OpenProjectTests.cstests/Beutl.HeadlessUITests/ShutdownPipelineTests.cstests/Beutl.HeadlessUITests/VersionControlRestoreTests.cstests/Beutl.HeadlessUITests/VersionControlSaveTests.cstests/Beutl.HeadlessUITests/VersionControlTabViewTests.cstests/Beutl.UnitTests/Editor/VersionControl/GitCliRunnerTests.cstests/Beutl.UnitTests/Editor/VersionControl/GitCliVersionControlServiceTests.cstests/Beutl.UnitTests/Editor/VersionControl/NestedRepositoryTests.cstests/Beutl.UnitTests/Editor/VersionControl/ProjectConflictMarkerScannerTests.cstests/Beutl.UnitTests/Editor/VersionControl/RealGitTestRepository.cstests/Beutl.UnitTests/Editor/VersionControl/RemoteOperationsTests.cstests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherStressTests.cstests/Beutl.UnitTests/Editor/VersionControl/RepositoryWatcherTests.cstests/Beutl.UnitTests/Editor/VersionControl/VersionControlModelsTests.cstests/Beutl.UnitTests/Editor/VersionControl/VersionControlPolicyTests.cstests/Beutl.UnitTests/Editor/VersionControl/VersionControlSnapshotScopeTests.cstests/Beutl.UnitTests/Editor/VersionControl/VersionControlTabViewModelTests.cstests/Beutl.UnitTests/Language/VersionControlStringsTests.cs
🚧 Files skipped from review as they are similar to previous changes (20)
- src/Beutl.Editor/Beutl.Editor.csproj
- src/Beutl.ProjectSystem/ProjectSystem/Scene.cs
- src/Beutl.Core/Project.cs
- src/Beutl/Views/MainView.axaml.InitializeMenuBar.cs
- src/Beutl/ViewModels/Dialogs/CreateNewProjectViewModel.cs
- src/Beutl.Editor/VersionControl/IProjectVersionControlCoordinator.cs
- src/Beutl.Language/Strings.resx
- tests/Beutl.UnitTests/Editor/VersionControl/VersionControlSnapshotScopeTests.cs
- tests/Beutl.UnitTests/Editor/VersionControl/RealGitTestRepository.cs
- docs/specs/005-project-git-versioning/contracts/coordinator-lifecycle.md
- tests/Beutl.UnitTests/Language/VersionControlStringsTests.cs
- tests/Beutl.UnitTests/Editor/VersionControl/NestedRepositoryTests.cs
- docs/specs/005-project-git-versioning/spec.md
- src/Beutl.Editor/VersionControl/IProjectVersionControlInitializer.cs
- docs/specs/005-project-git-versioning/quickstart.md
- src/Beutl.Editor/VersionControl/ProjectConflictMarkerScanner.cs
- src/Beutl.Language/Strings.ja.resx
- docs/specs/005-project-git-versioning/plan.md
- src/Beutl.Editor.Components/VersionControlTab/ViewModels/VersionControlTabViewModel.cs
- tests/Beutl.UnitTests/Editor/VersionControl/VersionControlTabViewModelTests.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a99a52f855
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/Beutl.HeadlessUITests/EditorProjectSessionGatewayTests.cs (1)
84-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport a descriptive failure when
WaitUntilAsynctimes out.On timeout,
Task.DelaythrowsTaskCanceledException. The failure message then names neither the test nor the awaited condition.VersionControlRestoreTests.WaitUntilAsyncends with an explicit assertion instead. Align this helper with that pattern so timeouts inMain_view_model_dispose_starts_teardown_of_an_open_editorandClose_then_open_waits_for_editor_teardown_before_returningare diagnosable.♻️ Proposed change
private static async Task WaitUntilAsync(Func<bool> condition) { - using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(5)); - while (!condition()) - { - await Task.Delay(10, timeout.Token); - } + var timeout = Stopwatch.StartNew(); + while (!condition() && timeout.Elapsed < TimeSpan.FromSeconds(5)) + { + await Task.Delay(10); + } + + Assert.That(condition(), Is.True, "The expected state was not reached."); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/EditorProjectSessionGatewayTests.cs` around lines 84 - 91, Update WaitUntilAsync to handle its five-second timeout and report a descriptive assertion failure identifying the awaited condition or calling tests, following the explicit assertion pattern used by VersionControlRestoreTests.WaitUntilAsync instead of allowing TaskCanceledException to escape.tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs (2)
3941-3985: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
CompletionCountto describe what it counts.
_completionCountis incremented only inside theOperationCanceledExceptionhandler on Line 3982, afterReleaseCancelledProbescompletes. It therefore counts cancelled probes that finished unwinding, not probes that completed successfully. The name misleads a reader of the assertion on Line 2569.Rename it to
CancelledProbeUnwindCountor similar.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs` around lines 3941 - 3985, Rename the BlockingGitInstallationProbe counter and property from CompletionCount/_completionCount to a name such as CancelledProbeUnwindCount that reflects cancelled probes finishing unwinding. Update the assertion and all references to use the new name, while preserving the existing increment behavior after ReleaseCancelledProbes completes.
3861-3862: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a platform-appropriate null device path for
GIT_CONFIG_GLOBAL.Three places hardcode
/dev/null. That path does not exist on Windows. Git treats a missingGIT_CONFIG_GLOBALfile as an empty config, so isolation still works today, but the literal is Unix-specific.FindGitOnPathon Line 3891 already branches onOperatingSystem.IsWindows(), so the file is otherwise platform-aware.Extract one shared constant that resolves to
NULon Windows and/dev/nullelsewhere, and use it inProbeGitOrIgnore,RunGitAsync, andIsolatedGitEnvironment.♻️ Proposed change
+ private static readonly string s_nullDevice = + OperatingSystem.IsWindows() ? "NUL" : "/dev/null";Then replace each
"/dev/null"literal withs_nullDevice.Also applies to: 3931-3932, 4443-4447
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs` around lines 3861 - 3862, Define a shared platform-aware null-device constant, such as s_nullDevice, resolving to NUL on Windows and /dev/null elsewhere. Replace the hardcoded literals in ProbeGitOrIgnore, RunGitAsync, and IsolatedGitEnvironment with this constant, reusing the existing OperatingSystem.IsWindows() pattern.tests/Beutl.HeadlessUITests/ShutdownPipelineTests.cs (1)
254-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNUnit assertions inside isolated lifecycle handlers cannot fail these tests.
ProjectServicedeliberately isolatesClosingFinalizinghandler exceptions, and both tests assert that isolation. AnAssert.Thatfailure inside such a handler throwsAssertionException, which the same isolation catches and logs. The cancellation-token checks are therefore unverifiable. Record each observed value in a local variable and assert it in the test body instead.
tests/Beutl.HeadlessUITests/ShutdownPipelineTests.cs#L254-L266: replace theAssert.That(cancellationToken, ...)calls infailingFinalizerandsubsequentFinalizerwith token captures, then assert both tokens in theAssert.Multipleblock on Lines 275-280.tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs#L2146-L2151: replace theAssert.That(cancellationToken, ...)call inobservingClosingFinalizerwith a token capture, then assert it in theAssert.Multipleblock on Lines 2159-2168.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/ShutdownPipelineTests.cs` around lines 254 - 266, Replace the NUnit assertions inside the isolated finalizer handlers with local cancellation-token captures. In tests/Beutl.HeadlessUITests/ShutdownPipelineTests.cs lines 254-266, update failingFinalizer and subsequentFinalizer, then assert both captured tokens in the existing Assert.Multiple block at lines 275-280; in tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs lines 2146-2151, update observingClosingFinalizer and assert its captured token in the existing Assert.Multiple block at lines 2159-2168.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl/AgentHost/AgentHostEndpoint.cs`:
- Around line 435-464: Bound the shutdown work in DrainCoreAsync by creating a
cancellation token with the configured drain timeout and passing it to
StopAndDisposeAsync instead of CancellationToken.None. Preserve the existing
lifecycle locking and ensure the token is disposed appropriately; rely on
StopAndDisposeAsync’s finally behavior to dispose the application even when the
drain deadline is reached.
In `@tests/Beutl.HeadlessUITests/ShutdownPipelineTests.cs`:
- Around line 186-187: Reset shell singletons as the first statement in the
affected Avalonia test body, before constructing ProjectService or calling
SetOpenProject. Use the existing TestReset.ResetShellAsync pattern demonstrated
by MainViewModel_shutdown_coalesces_close_handlers_and_releases_project, keeping
the reset inside the test rather than setup or teardown.
---
Nitpick comments:
In `@tests/Beutl.HeadlessUITests/EditorProjectSessionGatewayTests.cs`:
- Around line 84-91: Update WaitUntilAsync to handle its five-second timeout and
report a descriptive assertion failure identifying the awaited condition or
calling tests, following the explicit assertion pattern used by
VersionControlRestoreTests.WaitUntilAsync instead of allowing
TaskCanceledException to escape.
In `@tests/Beutl.HeadlessUITests/ShutdownPipelineTests.cs`:
- Around line 254-266: Replace the NUnit assertions inside the isolated
finalizer handlers with local cancellation-token captures. In
tests/Beutl.HeadlessUITests/ShutdownPipelineTests.cs lines 254-266, update
failingFinalizer and subsequentFinalizer, then assert both captured tokens in
the existing Assert.Multiple block at lines 275-280; in
tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs lines 2146-2151,
update observingClosingFinalizer and assert its captured token in the existing
Assert.Multiple block at lines 2159-2168.
In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs`:
- Around line 3941-3985: Rename the BlockingGitInstallationProbe counter and
property from CompletionCount/_completionCount to a name such as
CancelledProbeUnwindCount that reflects cancelled probes finishing unwinding.
Update the assertion and all references to use the new name, while preserving
the existing increment behavior after ReleaseCancelledProbes completes.
- Around line 3861-3862: Define a shared platform-aware null-device constant,
such as s_nullDevice, resolving to NUL on Windows and /dev/null elsewhere.
Replace the hardcoded literals in ProbeGitOrIgnore, RunGitAsync, and
IsolatedGitEnvironment with this constant, reusing the existing
OperatingSystem.IsWindows() pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e00a5d91-6101-417e-b536-d24b30fbdf06
📒 Files selected for processing (11)
src/Beutl/AgentHost/AgentHostEndpoint.cssrc/Beutl/Services/ProjectService.cssrc/Beutl/Services/VersionControlCoordinator.cssrc/Beutl/ViewModels/EditorHostViewModel.cssrc/Beutl/ViewModels/MainViewModel.cstests/Beutl.HeadlessUITests/AgentHostEndpointTests.cstests/Beutl.HeadlessUITests/EditorProjectSessionGatewayTests.cstests/Beutl.HeadlessUITests/ShutdownPipelineTests.cstests/Beutl.HeadlessUITests/TestReset.cstests/Beutl.HeadlessUITests/VersionControlRestoreTests.cstests/Beutl.HeadlessUITests/VersionControlTabViewTests.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Beutl.HeadlessUITests/TestReset.cs
- tests/Beutl.HeadlessUITests/VersionControlTabViewTests.cs
- src/Beutl/Services/VersionControlCoordinator.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff40b911ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/Beutl.UnitTests/Editor/VersionControl/GitCliVersionControlServiceTests.cs (1)
457-496: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match the exercised API and assert the recorded LFS calls.
The test name states
EnsureRepositoryHygieneAsync, but the body callsservice.InitializeAsync. The test also builds aRecordingLfsRunnerand never assertsLfsInstallCalls. Rename the test to describe initialization, or callEnsureRepositoryHygieneAsync. Add an assertion onrunner.LfsInstallCalls, or drop the recording runner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.UnitTests/Editor/VersionControl/GitCliVersionControlServiceTests.cs` around lines 457 - 496, Rename EnsureRepositoryHygieneAsync_keeps_user_Lfs_overrides_after_the_managed_block to describe the exercised InitializeAsync behavior, and add an assertion verifying the expected runner.LfsInstallCalls value. Keep the existing attribute-file and Git check-attr assertions unchanged.tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs (3)
2941-2948: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffPrefer an internal test hook over reflection on
_configurationActivationQuiesced.The test replaces a private field of
VersionControlCoordinatorthrough reflection. This couples the test to a private implementation detail and can leave the coordinator in a state the production code never produces. TheAssert.That(readinessField, Is.Not.Null)check catches a rename, but it does not protect against a change in how the field is used. Expose aninternalseam on the coordinator for the quiescence signal, and use that seam here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs` around lines 2941 - 2948, Replace the reflection-based assignment of _configurationActivationQuiesced in the affected test with an internal test hook on VersionControlCoordinator that exposes the quiescence signal for controlled setup. Update the test to assign synchronousReadiness through that hook, preserving the existing synchronization behavior without accessing private fields.
2685-2693: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCollect factory-created backends in a thread-safe collection.
createdBackendsis aList<PullCycleTestBackend>. The service factory runs on coordinator activation threads, and the test body readscreatedBackends.Countand enumerates the list from the test thread.List<T>is not safe for concurrent add and read, so this can produce a torn count or a stale view and make the assertions flaky. Use aConcurrentBag<PullCycleTestBackend>or guard the list with a lock. The same pattern appears at Line 2777 (publishedServices), Line 3103, and Line 3470.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs` around lines 2685 - 2693, Replace the thread-unsafe List collections used for createdBackends and the corresponding publishedServices, createdBackends, and publishedServices instances at the other occurrences with a thread-safe collection such as ConcurrentBag<T>. Update any affected initialization or enumeration code while preserving the existing factory additions and test assertions.
5254-5254: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
InitializationOptionsagainst concurrent access.
InitializeAsyncadds toInitializationOptions, and tests read the list by index from the test thread.List<T>does not support concurrent add and read. Use a lock or a concurrent collection so the identity-retry assertions stay deterministic.Also applies to: 5397-5397
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs` at line 5254, Protect the shared InitializationOptions collection used by InitializeAsync and the test assertions from concurrent access. Update the InitializationOptions declaration and every add/index-read in the affected test flows to use consistent synchronization or a suitable concurrent collection, while preserving deterministic identity-retry assertions at both referenced locations.tests/Beutl.UnitTests/Editor/VersionControl/NestedRepositoryTests.cs (1)
568-579: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the symbolic-link test helpers into the shared base fixture. Three fixtures that all derive from
RealGitTestRepositorynow declare identical platform-aware symlink helpers, so the sameAssert.Ignorepolicy is duplicated in three places and can drift.
tests/Beutl.UnitTests/Editor/VersionControl/NestedRepositoryTests.cs#L568-L579: removeCreateFileSymbolicLinkOrIgnoreand the neighbouringCreateDirectorySymbolicLinkOrIgnore, and call the base-class helpers instead.tests/Beutl.UnitTests/Editor/VersionControl/GitCliVersionControlServiceTests.cs#L2219-L2230: removeCreateFileSymbolicLinkOrIgnoreand call the base-class helper.tests/Beutl.UnitTests/Editor/VersionControl/GitCliRunnerTests.cs#L643-L656: removeCreateDirectorySymbolicLinkOrIgnoreand call the base-class helper. Add both helpers asprotected staticmembers ofRealGitTestRepository.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.UnitTests/Editor/VersionControl/NestedRepositoryTests.cs` around lines 568 - 579, Centralize the platform-aware symlink helpers in RealGitTestRepository as protected static members, preserving their existing Assert.Ignore behavior. In tests/Beutl.UnitTests/Editor/VersionControl/NestedRepositoryTests.cs:568-579, remove both local helpers and use the base helpers; in tests/Beutl.UnitTests/Editor/VersionControl/GitCliVersionControlServiceTests.cs:2219-2230, remove CreateFileSymbolicLinkOrIgnore and call the base helper; in tests/Beutl.UnitTests/Editor/VersionControl/GitCliRunnerTests.cs:643-656, remove CreateDirectorySymbolicLinkOrIgnore and call the base helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.Editor/VersionControl/GitCliVersionControlService.cs`:
- Around line 3589-3594: Update CommitAllCoreAsync and the related flow around
FindIgnoredExistingRequiredProjectPathAsync so the pre-commit ignore probe uses
only the bounded required hygiene paths and representative media paths produced
by GetRequiredProjectRelativePaths, rather than enumerating every file beneath
resources. Remove or bypass the full-tree EnumerateRequiredProjectFiles walk,
and ensure inaccessible directories cannot cause the save to fail; alternatively
cache its result and invalidate it through RepositoryWatcher.
---
Nitpick comments:
In `@tests/Beutl.HeadlessUITests/VersionControlRestoreTests.cs`:
- Around line 2941-2948: Replace the reflection-based assignment of
_configurationActivationQuiesced in the affected test with an internal test hook
on VersionControlCoordinator that exposes the quiescence signal for controlled
setup. Update the test to assign synchronousReadiness through that hook,
preserving the existing synchronization behavior without accessing private
fields.
- Around line 2685-2693: Replace the thread-unsafe List collections used for
createdBackends and the corresponding publishedServices, createdBackends, and
publishedServices instances at the other occurrences with a thread-safe
collection such as ConcurrentBag<T>. Update any affected initialization or
enumeration code while preserving the existing factory additions and test
assertions.
- Line 5254: Protect the shared InitializationOptions collection used by
InitializeAsync and the test assertions from concurrent access. Update the
InitializationOptions declaration and every add/index-read in the affected test
flows to use consistent synchronization or a suitable concurrent collection,
while preserving deterministic identity-retry assertions at both referenced
locations.
In
`@tests/Beutl.UnitTests/Editor/VersionControl/GitCliVersionControlServiceTests.cs`:
- Around line 457-496: Rename
EnsureRepositoryHygieneAsync_keeps_user_Lfs_overrides_after_the_managed_block to
describe the exercised InitializeAsync behavior, and add an assertion verifying
the expected runner.LfsInstallCalls value. Keep the existing attribute-file and
Git check-attr assertions unchanged.
In `@tests/Beutl.UnitTests/Editor/VersionControl/NestedRepositoryTests.cs`:
- Around line 568-579: Centralize the platform-aware symlink helpers in
RealGitTestRepository as protected static members, preserving their existing
Assert.Ignore behavior. In
tests/Beutl.UnitTests/Editor/VersionControl/NestedRepositoryTests.cs:568-579,
remove both local helpers and use the base helpers; in
tests/Beutl.UnitTests/Editor/VersionControl/GitCliVersionControlServiceTests.cs:2219-2230,
remove CreateFileSymbolicLinkOrIgnore and call the base helper; in
tests/Beutl.UnitTests/Editor/VersionControl/GitCliRunnerTests.cs:643-656, remove
CreateDirectorySymbolicLinkOrIgnore and call the base helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 461b4ac6-9c25-4b18-8afb-68e22877b0a5
📒 Files selected for processing (17)
docs/specs/005-project-git-versioning/research.mdsrc/Beutl.Core/Project.cssrc/Beutl.Core/ProjectItem.cssrc/Beutl.Editor/VersionControl/GitCliRunner.cssrc/Beutl.Editor/VersionControl/GitCliVersionControlService.cssrc/Beutl.Editor/VersionControl/VersionControlModels.cssrc/Beutl.ProjectSystem/ProjectSystem/Scene.cssrc/Beutl/Services/VersionControlCoordinator.cssrc/Beutl/ViewModels/MenuBarViewModel.Files.cstests/Beutl.E2ETests/Scenarios/ProjectPersistenceTests.cstests/Beutl.HeadlessUITests/ShutdownPipelineTests.cstests/Beutl.HeadlessUITests/VersionControlRestoreTests.cstests/Beutl.HeadlessUITests/VersionControlSaveTests.cstests/Beutl.HeadlessUITests/VersionControlTabViewTests.cstests/Beutl.UnitTests/Editor/VersionControl/GitCliRunnerTests.cstests/Beutl.UnitTests/Editor/VersionControl/GitCliVersionControlServiceTests.cstests/Beutl.UnitTests/Editor/VersionControl/NestedRepositoryTests.cs
🚧 Files skipped from review as they are similar to previous changes (10)
- src/Beutl.Core/ProjectItem.cs
- src/Beutl.ProjectSystem/ProjectSystem/Scene.cs
- docs/specs/005-project-git-versioning/research.md
- tests/Beutl.HeadlessUITests/VersionControlTabViewTests.cs
- src/Beutl.Core/Project.cs
- tests/Beutl.HeadlessUITests/ShutdownPipelineTests.cs
- src/Beutl/ViewModels/MenuBarViewModel.Files.cs
- src/Beutl.Editor/VersionControl/GitCliRunner.cs
- src/Beutl.Editor/VersionControl/VersionControlModels.cs
- src/Beutl/Services/VersionControlCoordinator.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b67633e6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d58e2ff669
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ce5fb2f10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dcdd3353c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c664b1e9a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f964e76dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d932f8e472
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b9bb2670f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fc3daedcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84531ec3d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tool tab Clicking "Enable Version Control…" in the tool tab raised the shell context command and returned immediately, because IContextCommandHandler.Execute is void. The button therefore snapped back to its idle state while the project was still being saved, initialized and committed, leaving no sign that anything was running. ContextCommandExecution now carries a Completion task and dispatches through AsyncReactiveCommand.ExecuteAsync, so a caller can await the work a handler started; MainViewModel publishes it and the tab awaits it. The tab view model exposes the resulting running state, which drives an indeterminate ring, a "Enabling version control…" label and a hint that a large project takes a while — and keeps the button disabled for the whole operation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f736589760
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nd completion The completion helper on ContextCommandExecution recognized only the concrete non-generic AsyncReactiveCommand, so a plugin handing it an AsyncReactiveCommand<T> — or any other asynchronous ICommand — would have had its work reported as already finished. Drop the helper: the public contract is now just the Task a handler publishes, which fits every implementation. MainViewModel keeps the AsyncReactiveCommand dispatch, where the command set is its own MenuBar and the types are known.
…s is cancelled Cancellation was excluded from the rollback path, so a cancel landing between `git rm --cached` and its commit left the reserved .beutl/*.tmp entries staged as deletions. The next user commit would then stop tracking them without anyone asking for it. The compensating restore now runs on the cancellation path too — with its own token, since it has to complete either way — before the cancellation surfaces.
…r the identity prompt The editor stays live while the identity prompt is open, so edits made while the user types their name and email were still in memory when the retry committed — and the manual version was reported as created without them. The retry now re-saves the open project first, the same way the initial attempt does.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45893fe9f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… without asking again Only the non-nested path consulted the durable opt-in, so a project living inside an enclosing repository re-asked "use the enclosing repository?" on every open. Dismissing or declining it stopped that session from publishing a tracked backend, and explicit saves and project close silently produced no snapshots even though the project's opt-in was already recorded. Both the opening preflight and activation now check HasVersionTrackingOptInAsync first and prompt only for a repository this project has not been adopted into yet, matching the non-nested rule.
…am commit The prefetch before a fast-forward pull named the local branch, whose tip the preflight's fetch does not move — only the remote-tracking ref advances. git-lfs resolves that name locally, so the objects introduced upstream were never downloaded and the checkout pulled them on the uncancellable path, after the project had already closed. PullPreflightResult now carries the upstream commit it verified, and the pull prefetches that commit through the existing commit-scoped prefetch, the same way restore does.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96526687ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rktree transitions A repository can restrict which LFS paths are hydrated through lfs.fetchinclude and lfs.fetchexclude. The transition checkouts, the branch switches and the prefetch that feeds them all inherited those filters, and git-lfs copies an excluded pointer through unchanged — so a restore, pull or branch switch could reopen the project with pointer text where its media belongs, including for files the user had hydrated by hand. Every worktree-mutating command Beutl runs now clears both filters, so a transition always lands on real media. The runner fakes in the tests matched the git subcommand by argument position; they now skip the `-c` overrides that precede it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 600c18f901
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The cycle created the branch at the selected commit, so `git switch -c` checked that whole tree out. In an adopted enclosing repository this rolled back tracked files outside the project and could overwrite an ignored file colliding with a path the old commit tracks — while a plain restore stays pathspec-scoped through CommitProjectTreeAsync. The branch is now created at the current tip and the selected project tree is applied on it, so both restore shapes touch the same files.
A version-control transition suspends the editors from before its pre-transition save until the project closes, but the live binding reported the session alive whenever a scene existed. An agent edit accepted in that window reached only the in-memory scene: autosave waits behind the worktree lease and is cancelled by editor disposal, so the close discarded it silently. Liveness now follows the editor's enabled state, which is what the transition clears, so live mutations are rejected for exactly as long as UI input is.
…esystem's case rule A case-insensitive volume resolves `.BEUTL` to Beutl's reserved state directory, but the ordinal segment match did not exclude it, so its view-state and autosave writes kept waking the watcher and its status/history refresh pipeline for files no snapshot ever records. The reserved segments now compare through FileSystemPathComparison, the same split the rest of the path handling uses.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 699b67751c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ft objects uncached The prefetch swallowed every GitOperationException, so an offline endpoint, a failed authentication or a missing object counted as success. The caller then closed the project and ran its checkout with CancellationToken.None, where the smudge filter starts its own unbounded download — exactly the wait the prefetch exists to move ahead of the close. A failure is now absorbed only when every LFS object the target revision needs is already in the local object store, which keeps offline transitions working on cached media; anything that cannot be proven present aborts the transition while it is still cancellable and the project is still open.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2914e87096
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…Git stderr A push drains sideband progress for the whole operation and every chunk was also appended to the buffer kept for failure classification, so a long transfer — or a remote that simply keeps talking — grew it without any limit. Progress reporting is unchanged; the retained diagnostic is now capped at its last 64 KiB, which is where Git prints the error the result is classified by.
… change The metadata watcher accepted `info/exclude` but not `info/attributes`, whose entries outrank every .gitattributes file. An external tool changing `text`, `eol` or a filter there can make project paths modified immediately, while the version control tab kept showing the previous clean state until some unrelated watched event arrived.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60ba1e8a81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A package export only took an output lease, which excludes version-control transitions but not saves, and project-file writers never waited for output leases either. The copy could therefore walk the project directory while a save was writing the project file and its sidecars, packaging files from different save points. The export now takes one reservation that covers both: the output lease first, so no worktree mutation can start and the wait stays bounded, then the project-file write reservation for the copy interval.
…les silently Branch operations are repository-wide by design, but Git overwrites ignored files during a switch by default, so in an adopted enclosing repository the switch could destroy files the project never tracked without saying anything. Both switch shapes now pass --no-overwrite-ignore, which makes Git refuse the collision instead, and the confirmation tells a project sharing someone else's repository that the switch reaches past its own directory before the decision is taken.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa3c6fa87b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sition Removing the Project menu shifted every root index, so the wiring took Tools as the View menu and Scene as the Tools menu. Reaching into the wrong menu threw, the surrounding catch swallowed it, and macOS silently lost its editor-tab, tool-tab, tool-window and dock-layout-preset entries. The menus are now looked up by header, which is what the wiring actually means and what survives the next menu edit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c9f5abdfa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… which utility Two independent limits in the Git process layer. A stderr record that never reaches a CR/LF was buffered whole, so a transport helper writing megabytes without a delimiter handed the caller one huge string even though the retained diagnostic was already capped; the in-progress record is now reported and cleared every 4 KiB while draining continues. PATH discovery shelled out to which/where.exe, so a minimal Linux image that carries git without those utilities reported no candidates and disabled version control for a Git that works. PATH is now searched in-process, with PATHEXT honoured on Windows.
…apshots and the branch check Three places assumed a project is only what the built-in file names describe. The conflict scan filtered by .bep/.scene/.belm, so a merge-conflicted sidecar an extension persists under any other name reached restoration as a JSON parse failure with no conflict guidance. Snapshots excluded every *.tmp path, which drops a required sidecar an extension persists under that name and reopens a revision with mismatched project data. Both now consult the paths the serialized project actually references, extracted into SerializedProjectGraph so the scan, the layout validation and the snapshot exclusion share one definition. The snapshot commit also never confirmed that the branch it captured still owned HEAD, so an external Git client switching branches mid-snapshot put the commit on an unrelated branch and reported success. The captured branch and the tip the snapshot was built on are verified after the commit.
…nd surface an unguarded output The pre-close save aborts the close on purpose so unsaved edits stay in their editors, but application shutdown reduced that to a warning and went on to dispose the composition and close the window — discarding exactly what the abort protected. The abort now travels as ProjectCloseAbortedException: shutdown stops before composition disposal, clears the otherwise terminal shutdown request so the user can save and close again, and the window coordinator leaves the window open and lets a later attempt retry. An output context that starts before raising Started can also lose the fallback workspace reservation. It cannot be stopped from there — IOutputContext has no cancellation contract — so the user is now told the output is running while the worktree changes instead of only the log knowing.
|
No TODO comments were found. |
Minimum allowed line rate is |
Description
Affected areas
Beutl.Engine(rendering / scene / animation / media)Beutl.ProjectSystem(project / document persistence)Beutl.Editor,Beutl.Editor.Components,Beutl.Controls)Beutl.Extensibility(extensions / plugins)Beutl.NodeGraphBeutl.FFmpegIpc/Beutl.FFmpegWorkerBeutl.ApiBeutl.Core,Beutl.Configuration,Beutl.Language)Breaking changes
Project.Serializenow preserves persistedappVersionandminAppVersionduring a plain save instead of always stamping the running Beutl version. Migration code advances these fields only after rewriting persisted content.IProjectVersionControlServiceis now query-only and no longer exposes mutation or disposal operations. Mutations are coordinated throughIProjectVersionControlCoordinator; the backend and transaction surfaces are internal.IProjectVersionControlInitializer.InitializeCurrentProjectAsyncnow acceptsFunc<CancellationToken, Task<GitIdentity?>>, and the exact operation token is forwarded to the callback.CommitResult.Committednow carries aCommitRevision(KnownorUnavailable) so a durable commit is not reported as failed when its SHA cannot be observed afterward.SnapshotKind.RecoveryandRemoteOpResult.RepositoryDirtydistinguish compensating commits and actual cleanliness-precondition failures.VersionControlPolicyNoticeis now an internal discriminated union.MenuBarViewModelnow requires aVersionControlCoordinatorconstructor argument.CreateNewProjectViewModelnow accepts theIProjectVersionControlInitializerabstraction for Git availability and project initialization.Beutl.AgentToolkit:RenderJobManagerimplementsIAsyncDisposableinstead ofIDisposable, so callers must switch fromusing/Dispose()toawait using/DisposeAsync().Enqueuenow requires anOutputOperationLease(IDisposable) argument, which the caller obtains fromIOutputOperationLeaseProvider.TryBeginOutputOperation()and hands to the job.IProjectVersionControlSession.NotifySavedAsynctakes an optionalIProjectFileWriteLeasebefore itsCancellationToken, so a positional caller passing a token must name the argument.Strings.VersionControl_ExportInProgressis replaced byStrings.VersionControl_WorkspaceBusy.Beutl.Extensibility:OutputExtension.TryCreateContexttakes an additional parameter, so every out-of-treeOutputExtensionsubclass must update its override before it compiles.Test plan
dotnet format Beutl.slnx --no-restore --verify-no-changes(3,269 files checked, no formatting changes).claude/scripts/check-gpl-mit-boundary-diff.sh --files <all changed files>dotnet build Beutl.slnx -c Debug --no-restore(0 errors; 64 existing warnings)dotnet test Beutl.slnx -m:1 -c Debug -f net10.0 --no-build --settings coverlet.runsettings --logger "console;verbosity=normal"Fixed issues / References
Summary by CodeRabbit
BREAKING CHANGE:
Beutl.Extensibility:OutputExtension.TryCreateContexttakes an additionalIOutputOperationLeaseProviderparameter, so every out-of-treeOutputExtensionsubclass must update its override;IProjectVersionControlServiceis query-only, with mutations behindIProjectVersionControlCoordinatorand the backend/transaction surfaces internal;IProjectVersionControlInitializer.InitializeCurrentProjectAsynctakesFunc<CancellationToken, Task<GitIdentity?>>;IProjectVersionControlSession.NotifySavedAsynctakes an optionalIProjectFileWriteLeasebefore itsCancellationToken;CommitResult.Committedcarries aCommitRevision;VersionControlPolicyNoticeis internal;Strings.VersionControl_ExportInProgressis replaced byStrings.VersionControl_WorkspaceBusy.Beutl.AgentToolkit:RenderJobManagerimplementsIAsyncDisposableinstead ofIDisposableandEnqueuerequires anOutputOperationLease.Beutl:MenuBarViewModelrequires aVersionControlCoordinatorargument andCreateNewProjectViewModelrequiresIProjectVersionControlInitializer.Beutl.Core:Project.Serializepreserves persistedappVersion/minAppVersionon a plain save and project JSON is normalized to LF on every platform. See the "Breaking changes" section above for the per-item migration notes.