fix: designer - improved git behavior when there is concurrent work on the same branch #18033
Open
martinothamar wants to merge 8 commits intomainfrom
Open
fix: designer - improved git behavior when there is concurrent work on the same branch #18033martinothamar wants to merge 8 commits intomainfrom
martinothamar wants to merge 8 commits intomainfrom
Conversation
github-actions
Bot
added
area/version-control
Contributor
|
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:
📝 WalkthroughWalkthroughSwitches NonFastForward recovery from pull-then-push to a rebase-based workflow, adds Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Controller as RepositoryController
participant SourceControl as SourceControlService
participant GitLib as LibGit2Sharp
Client->>Controller: POST /commit-and-push (commit + push)
Controller->>SourceControl: CommitAndPushToBranch(request)
alt Push succeeds
SourceControl-->>Controller: Success
Controller-->>Client: 200 OK
else Push fails (NonFastForward)
Controller->>SourceControl: Resolve branchName
Controller->>SourceControl: RebaseOntoRemoteBranch(branchName)
SourceControl->>GitLib: fetch origin + notes, start rebase onto refs/remotes/origin/branchName
alt RebaseStatus = Conflicts
GitLib-->>SourceControl: RebaseResult(Conflicts)
SourceControl-->>Controller: throw NonFastForwardException (conflict)
Controller-->>Client: conflict response (error)
else RebaseStatus = Success
GitLib-->>SourceControl: RebaseResult(Success, ContentStatus)
SourceControl->>SourceControl: PublishBranch(branchName)
SourceControl-->>Controller: RemoteRebaseResult with ContentStatus
Controller->>Client: emit FileSyncSuccess per file
Controller-->>Client: 200 OK
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
martinothamar
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.tsx (1)
100-101: ExtractforceRepoStatusCheckinto a shared helper.This logic is duplicated here and in
VersionControlButtonsContext.tsx; keeping one implementation avoids future drift.♻️ Suggested refactor
-const forceRepoStatusCheck = () => - window.postMessage(postMessages.forceRepoStatusCheck, window.location.href); +import { forceRepoStatusCheck } from 'app-shared/utils/forceRepoStatusCheck';// app-shared/utils/forceRepoStatusCheck.ts import postMessages from 'app-shared/utils/postMessages'; export const forceRepoStatusCheck = () => window.postMessage(postMessages.forceRepoStatusCheck, window.location.href);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.tsx` around lines 100 - 101, Extract the duplicated forceRepoStatusCheck function into a single shared helper (e.g., app-shared/utils/forceRepoStatusCheck.ts) that imports postMessages from app-shared/utils/postMessages and exports the same function signature; then replace the local const forceRepoStatusCheck definitions in FetchChangesPopover (component) and VersionControlButtonsContext (module) with imports from the new helper so both use the identical implementation.src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs (1)
1470-1644: Split the auto-stash recovery method and extract status literals.
TryRecoverPullCheckoutConflictWithAutoStashcurrently combines stash creation, pull, apply, rollback, exception mapping, and telemetry string selection. Breaking it into smaller helpers and centralising status keys will reduce maintenance risk and typo drift.As per coding guidelines, `src/Designer/**/*.{js,jsx,ts,tsx,cs}`: “Avoid hard-coded numbers and strings” and “Write short functions that only do one thing”.🧩 Suggested direction
+private static class PullRecoveryStatusKeys +{ + public const string CheckoutConflict = "checkout_conflict"; + public const string PullConflict = "pull_conflict"; + public const string ApplyConflict = "pull_auto_stash_apply_conflict"; + public const string PullFailed = "pull_auto_stash_pull_failed"; + public const string RestoreFailed = "pull_auto_stash_restore_failed"; +}// Suggested decomposition // 1) CreateAutoStash(...) // 2) PullDuringRecovery(...) // 3) ApplyAutoStash(...) // 4) HandleRecoveryFailure(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs` around lines 1470 - 1644, Split TryRecoverPullCheckoutConflictWithAutoStash into small focused helpers: extract stash creation to CreateAutoStash(repo, authenticatedContext) that returns stashIndex, move the pull logic to PullDuringRecovery(repo, authenticatedContext, pullOptions, headCommitBeforePull, recoveryStatus) returning (mergeStatus, pullConflict, mergeConflictCount), move stash apply/inspection to ApplyAutoStash(repo, stashIndex) returning whether apply conflict occurred, and centralize rollback/exception handling into HandleRecoveryFailure(repo, headCommitBeforePull, authenticatedContext, stashIndex, applyStarted, exOrReason) which performs RollbackAndRestoreAutoStash and maps telemetry/status strings; also extract all literal status keys ("checkout_conflict", "pull_conflict", "pull_auto_stash_apply_conflict", "pull_auto_stash_pull_failed", "pull_auto_stash_failed", "apply_conflict", "apply_exception", etc.) into a single static class/enum (e.g., AutoStashStatusKeys) and use those constants everywhere in TryRecoverPullCheckoutConflictWithAutoStash and the new helpers to remove hard-coded strings and shorten the original method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs`:
- Around line 1470-1644: Split TryRecoverPullCheckoutConflictWithAutoStash into
small focused helpers: extract stash creation to CreateAutoStash(repo,
authenticatedContext) that returns stashIndex, move the pull logic to
PullDuringRecovery(repo, authenticatedContext, pullOptions,
headCommitBeforePull, recoveryStatus) returning (mergeStatus, pullConflict,
mergeConflictCount), move stash apply/inspection to ApplyAutoStash(repo,
stashIndex) returning whether apply conflict occurred, and centralize
rollback/exception handling into HandleRecoveryFailure(repo,
headCommitBeforePull, authenticatedContext, stashIndex, applyStarted,
exOrReason) which performs RollbackAndRestoreAutoStash and maps telemetry/status
strings; also extract all literal status keys ("checkout_conflict",
"pull_conflict", "pull_auto_stash_apply_conflict",
"pull_auto_stash_pull_failed", "pull_auto_stash_failed", "apply_conflict",
"apply_exception", etc.) into a single static class/enum (e.g.,
AutoStashStatusKeys) and use those constants everywhere in
TryRecoverPullCheckoutConflictWithAutoStash and the new helpers to remove
hard-coded strings and shorten the original method.
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.tsx`:
- Around line 100-101: Extract the duplicated forceRepoStatusCheck function into
a single shared helper (e.g., app-shared/utils/forceRepoStatusCheck.ts) that
imports postMessages from app-shared/utils/postMessages and exports the same
function signature; then replace the local const forceRepoStatusCheck
definitions in FetchChangesPopover (component) and VersionControlButtonsContext
(module) with imports from the new helper so both use the identical
implementation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Designer/backend/src/Designer/Controllers/RepositoryController.cssrc/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cssrc/Designer/backend/src/Designer/Services/Interfaces/ISourceControl.cssrc/Designer/backend/tests/Designer.Tests/Mocks/ISourceControlMock.cssrc/Designer/backend/tests/Designer.Tests/Services/SourceControlServiceTest.cssrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18033 +/- ##
==========================================
- Coverage 95.20% 95.19% -0.01%
==========================================
Files 2505 2519 +14
Lines 32588 31942 -646
Branches 3869 3900 +31
==========================================
- Hits 31025 30407 -618
+ Misses 1218 1207 -11
+ Partials 345 328 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
martinothamar
force-pushed
the
fix/pull-conflict-issue
branch
2 times, most recently
from
f4368f0 to
3e59d83
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsx (1)
131-157: Makewindow.postMessagespy cleanup failure-safe.If an assertion fails before Line 156, the global spy can leak into later tests. Use a
try/finally(orafterEach(jest.restoreAllMocks)).Suggested test hardening
- const postMessageSpy = jest.spyOn(window, 'postMessage').mockImplementation(); + const postMessageSpy = jest.spyOn(window, 'postMessage').mockImplementation(() => undefined); @@ - await waitFor(() => { - expect(mockVersionControlButtonsContextValue.setHasMergeConflict).toHaveBeenCalledWith(true); - }); - expect(postMessageSpy).toHaveBeenCalledWith( - postMessages.forceRepoStatusCheck, - window.location.href, - ); - expect(mockVersionControlButtonsContextValue.commitAndPushChanges).not.toHaveBeenCalled(); - postMessageSpy.mockRestore(); + try { + await waitFor(() => { + expect(mockVersionControlButtonsContextValue.setHasMergeConflict).toHaveBeenCalledWith(true); + }); + expect(postMessageSpy).toHaveBeenCalledWith( + postMessages.forceRepoStatusCheck, + window.location.href, + ); + expect(mockVersionControlButtonsContextValue.commitAndPushChanges).not.toHaveBeenCalled(); + } finally { + postMessageSpy.mockRestore(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsx` around lines 131 - 157, The test creates a global spy postMessageSpy via jest.spyOn(window, 'postMessage') but only calls postMessageSpy.mockRestore() at the end, which can leak when an assertion fails; fix by making the spy cleanup failure-safe — either add a top-level afterEach(() => jest.restoreAllMocks()) in this test file or wrap the test body that creates postMessageSpy (the code around postMessageSpy, renderFetchChangesPopover, user.click(fetchButton) and assertions) in a try/finally and call postMessageSpy.mockRestore() in the finally block so the spy is always restored; reference the postMessageSpy identifier and the renderFetchChangesPopover/test block when applying the change.src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs (1)
1470-1635: Extract status literals and split the auto-stash recovery flow into smaller helpers.This method currently bundles stash creation, pull, stash apply, rollback, and status/error classification, with repeated string literals. Extracting constants and a couple of focused helpers would reduce drift and make the failure paths easier to maintain.
♻️ Example extraction
+ private const string MergeStatusCheckoutConflict = "checkout_conflict"; + private const string ErrorPullConflict = "pull_conflict"; + private const string ErrorAutoStashApplyConflict = "pull_auto_stash_apply_conflict"; + private const string ErrorAutoStashPullFailed = "pull_auto_stash_pull_failed"; + - string mergeStatus = "checkout_conflict"; + string mergeStatus = MergeStatusCheckoutConflict; ... - SetErrorStatus(methodActivity, "pull_conflict"); + SetErrorStatus(methodActivity, ErrorPullConflict);As per coding guidelines:
src/Designer/**/*.{js,jsx,ts,tsx,cs}: "Avoid hard-coded numbers and strings" and "Write short functions that only do one thing".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs` around lines 1470 - 1635, The TryRecoverPullCheckoutConflictWithAutoStash method is too large and contains repeated status string literals; refactor by extracting the status strings (e.g., "checkout_conflict", "pull_conflict", "pull_auto_stash_apply_conflict", "pull_auto_stash_pull_failed", "pull_auto_stash_failed") into named constants and split the flow into small helpers such as CreateAutoStashAndMarkIndex(repo, authenticatedContext) to add the stash and return the stash index, ExecutePullWithRecovery(repo, authenticatedContext, pullOptions, headCommitBeforePull, recoveryStatus) to run PullAndCollectStatus and return (mergeStatus, pullConflict, mergeConflictCount), and ApplyAutoStashAndHandleConflicts(repo, stashIndex, headCommitBeforePull, authenticatedContext, methodActivity, recoveryStatus) to apply the stash, detect conflicts, call RollbackAndRestoreAutoStash when needed and update status; keep exception handling centralized so callers receive AutoStashRecoveryResult with proper flags and reuse RollbackAndRestoreAutoStash for rollback logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs`:
- Around line 977-984: The handler in RebaseOntoRemoteBranch currently throws
InvalidOperationException when rebase is stopped (RebaseStatus.Stop), which
bypasses the controller's NonFastForwardException handling; change the thrown
exception to NonFastForwardException (preserving the existing message and rebase
abort/SetErrorStatus behavior) so PushChangesForRepository and the controller's
filters treat this as a conflict; locate the RebaseStatus.Stop branch where
repo.Rebase.Abort(), stopAborted, SetErrorStatus(ctx.activity, "rebase_stopped")
are set and replace the InvalidOperationException creation with a
NonFastForwardException containing the same diagnostic text.
---
Nitpick comments:
In
`@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs`:
- Around line 1470-1635: The TryRecoverPullCheckoutConflictWithAutoStash method
is too large and contains repeated status string literals; refactor by
extracting the status strings (e.g., "checkout_conflict", "pull_conflict",
"pull_auto_stash_apply_conflict", "pull_auto_stash_pull_failed",
"pull_auto_stash_failed") into named constants and split the flow into small
helpers such as CreateAutoStashAndMarkIndex(repo, authenticatedContext) to add
the stash and return the stash index, ExecutePullWithRecovery(repo,
authenticatedContext, pullOptions, headCommitBeforePull, recoveryStatus) to run
PullAndCollectStatus and return (mergeStatus, pullConflict, mergeConflictCount),
and ApplyAutoStashAndHandleConflicts(repo, stashIndex, headCommitBeforePull,
authenticatedContext, methodActivity, recoveryStatus) to apply the stash, detect
conflicts, call RollbackAndRestoreAutoStash when needed and update status; keep
exception handling centralized so callers receive AutoStashRecoveryResult with
proper flags and reuse RollbackAndRestoreAutoStash for rollback logic.
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsx`:
- Around line 131-157: The test creates a global spy postMessageSpy via
jest.spyOn(window, 'postMessage') but only calls postMessageSpy.mockRestore() at
the end, which can leak when an assertion fails; fix by making the spy cleanup
failure-safe — either add a top-level afterEach(() => jest.restoreAllMocks()) in
this test file or wrap the test body that creates postMessageSpy (the code
around postMessageSpy, renderFetchChangesPopover, user.click(fetchButton) and
assertions) in a try/finally and call postMessageSpy.mockRestore() in the
finally block so the spy is always restored; reference the postMessageSpy
identifier and the renderFetchChangesPopover/test block when applying the
change.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6766c187-479f-4d13-b287-448adb0d2187
📒 Files selected for processing (8)
src/Designer/backend/src/Designer/Controllers/RepositoryController.cssrc/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cssrc/Designer/backend/src/Designer/Services/Interfaces/ISourceControl.cssrc/Designer/backend/tests/Designer.Tests/Mocks/ISourceControlMock.cssrc/Designer/backend/tests/Designer.Tests/Services/SourceControlServiceTest.cssrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Designer/backend/tests/Designer.Tests/Mocks/ISourceControlMock.cs
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx
- src/Designer/backend/src/Designer/Services/Interfaces/ISourceControl.cs
mirkoSekulic
reviewed
Mar 4, 2026
martinothamar
force-pushed
the
fix/pull-conflict-issue
branch
from
87e5420 to
6f08374
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx (1)
59-70:⚠️ Potential issue | 🟠 Major
isLoadingcan stay stuck after a failed push in non-conflict paths.The catch path only resets loading when a conflict is detected, and
resultis dereferenced without a null check. A failed refetch or a non-conflict response can leave the UI in a perpetual loading state.Proposed fix
const commitAndPushChanges = async (commitMessage: string) => { setIsLoading(true); try { await repoCommitAndPushMutation({ commitMessage }); } catch (error) { console.error(error); const { data: result } = await fetchPullData(); - if ( - result.hasMergeConflict || - result.repositoryStatus === 'MergeConflict' || - result.repositoryStatus === 'CheckoutConflict' - ) { + if ( + result && + ( + result.hasMergeConflict || + result.repositoryStatus === 'MergeConflict' || + result.repositoryStatus === 'CheckoutConflict' + ) + ) { const conflictStatus: RepoStatus = { ...result, hasMergeConflict: true }; queryClient.setQueryData([QueryKey.RepoStatus, owner, repoName], conflictStatus); - setIsLoading(false); setHasMergeConflict(true); } return; + } finally { + setIsLoading(false); } const { data: result } = await fetchPullData(); if (result.repositoryStatus === 'Ok') { toast.success(t('sync_header.sharing_changes_completed'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx` around lines 59 - 70, The code dereferences result and unconditionally returns, causing isLoading to remain true on non-conflict or failed fetches; update the fetchPullData handling in VersionControlButtonsContext so you null-check result before using it, only perform the conflict branch and early return when result exists and indicates a conflict (checking result.hasMergeConflict or repositoryStatus), and in all other cases ensure setIsLoading(false) is called and setHasMergeConflict(false) is set; also wrap the fetchPullData call in a try/catch so any thrown error also calls setIsLoading(false) (and optionally setHasMergeConflict(false)) instead of leaving the UI stuck.
🧹 Nitpick comments (2)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx (1)
62-63: Replace repository-status string literals with shared constants/enums.The new
'MergeConflict'and'CheckoutConflict'literals are brittle and easy to drift from backend values. Prefer a shared status enum/constant map here.As per coding guidelines,
src/Designer/**/*.{js,jsx,ts,tsx,cs}: Avoid hard-coded numbers and strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx` around lines 62 - 63, The condition in VersionControlButtonsContext checks result.repositoryStatus against hard-coded strings 'MergeConflict' and 'CheckoutConflict'; replace these literals with the shared repository status constants/enum used across the app (import the enum/const map and compare result.repositoryStatus to e.g. RepositoryStatus.MergeConflict and RepositoryStatus.CheckoutConflict) so the checks use the canonical values instead of brittle strings; update the import and all references in VersionControlButtonsContext to use the shared symbol names (result.repositoryStatus, RepositoryStatus) accordingly.src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs (1)
1470-1635: Auto-stash recovery flow is too complex for one method.
TryRecoverPullCheckoutConflictWithAutoStashcurrently owns stash lifecycle, pull, apply, rollback, status shaping, and exception classification. Splitting this into smaller helpers would make failure semantics easier to reason about and maintain.As per coding guidelines,
src/Designer/**/*.{js,jsx,ts,tsx,cs}: Write short functions that only do one thing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs` around lines 1470 - 1635, Summary: TryRecoverPullCheckoutConflictWithAutoStash is too large and should be split into focused helpers. Refactor by extracting the stash lifecycle and pull/apply steps into small functions: create a StashChanges(authenticatedContext, repo) helper that wraps repo.Stashes.Add and returns stashIndex; create a TryAutoStashPull(repo, authenticatedContext, pullOptions, headCommitBeforePull) that calls PullAndCollectStatus and returns (mergeStatus, pullConflictDuringRecovery, mergeConflictCount, recoveryStatus); create ApplyAutoStash(repo, stashIndex) that applies the stash, checks repo.Index.Conflicts and removes the stash on success; preserve RollbackAndRestoreAutoStash usage for rollback paths. Then make TryRecoverPullCheckoutConflictWithAutoStash orchestrate these helpers, keeping the same return values and exception semantics (including the InvalidOperationException rethrow), and ensure SetErrorStatus and methodActivity exception logging remain in the same decision points so external behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx`:
- Around line 59-70: The code dereferences result and unconditionally returns,
causing isLoading to remain true on non-conflict or failed fetches; update the
fetchPullData handling in VersionControlButtonsContext so you null-check result
before using it, only perform the conflict branch and early return when result
exists and indicates a conflict (checking result.hasMergeConflict or
repositoryStatus), and in all other cases ensure setIsLoading(false) is called
and setHasMergeConflict(false) is set; also wrap the fetchPullData call in a
try/catch so any thrown error also calls setIsLoading(false) (and optionally
setHasMergeConflict(false)) instead of leaving the UI stuck.
---
Nitpick comments:
In
`@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs`:
- Around line 1470-1635: Summary: TryRecoverPullCheckoutConflictWithAutoStash is
too large and should be split into focused helpers. Refactor by extracting the
stash lifecycle and pull/apply steps into small functions: create a
StashChanges(authenticatedContext, repo) helper that wraps repo.Stashes.Add and
returns stashIndex; create a TryAutoStashPull(repo, authenticatedContext,
pullOptions, headCommitBeforePull) that calls PullAndCollectStatus and returns
(mergeStatus, pullConflictDuringRecovery, mergeConflictCount, recoveryStatus);
create ApplyAutoStash(repo, stashIndex) that applies the stash, checks
repo.Index.Conflicts and removes the stash on success; preserve
RollbackAndRestoreAutoStash usage for rollback paths. Then make
TryRecoverPullCheckoutConflictWithAutoStash orchestrate these helpers, keeping
the same return values and exception semantics (including the
InvalidOperationException rethrow), and ensure SetErrorStatus and methodActivity
exception logging remain in the same decision points so external behavior is
unchanged.
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx`:
- Around line 62-63: The condition in VersionControlButtonsContext checks
result.repositoryStatus against hard-coded strings 'MergeConflict' and
'CheckoutConflict'; replace these literals with the shared repository status
constants/enum used across the app (import the enum/const map and compare
result.repositoryStatus to e.g. RepositoryStatus.MergeConflict and
RepositoryStatus.CheckoutConflict) so the checks use the canonical values
instead of brittle strings; update the import and all references in
VersionControlButtonsContext to use the shared symbol names
(result.repositoryStatus, RepositoryStatus) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a2c38c2-bb33-4776-bb70-4a2cb07adf6a
📒 Files selected for processing (6)
src/Designer/backend/src/Designer/Controllers/RepositoryController.cssrc/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cssrc/Designer/backend/src/Designer/Services/Interfaces/ISourceControl.cssrc/Designer/backend/tests/Designer.Tests/Mocks/ISourceControlMock.cssrc/Designer/backend/tests/Designer.Tests/Services/SourceControlServiceTest.cssrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx
mirkoSekulic
approved these changes
Mar 4, 2026
martinothamar
force-pushed
the
fix/pull-conflict-issue
branch
from
6f08374 to
4598f8d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs (1)
1524-1689: Auto-stash recovery logic is thorough but complex.The recovery flow handles multiple failure scenarios:
- Pull conflicts after stash → rollback
- Stash apply conflicts → rollback
- Exceptions during any step → attempt rollback
Consider extracting the repeated rollback-and-return pattern into a helper if this grows further, though the current implementation is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs` around lines 1524 - 1689, The TryRecoverPullCheckoutConflictWithAutoStash method repeats the same rollback-and-return logic in several places (on pull conflict, apply conflict, and in the exception handler) — extract that into a small helper that accepts the repo, headCommitBeforePull, authenticatedContext, stashIndex, a rollbackReason string, a flag indicating whether applyStarted, the current mergeStatus and mergeConflictCount, and the Activity so it can call SetErrorStatus, invoke RollbackAndRestoreAutoStash, throw an InvalidOperationException if rollback fails, and return the appropriate AutoStashRecoveryResult; then replace each duplicated block in TryRecoverPullCheckoutConflictWithAutoStash with calls to this new helper to reduce duplication and centralize error/rollback handling.src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx (1)
60-66: Extract the conflict-state update into a shared helper.This branch now mirrors the logic in
FetchChangesPopover, including the status literals and the cache write. Pulling that into a small helper keeps the two flows aligned when conflict handling changes again.As per coding guidelines,
Avoid hard-coded numbers and stringsandWrite short functions that only do one thing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx` around lines 60 - 66, Extract the duplicate conflict-state update into a small shared helper (e.g., updateConflictStatus or setRepoConflictStatus) that takes the queryClient, owner, repoName and the original result (RepoStatus) and performs the hasMergeConflict merge plus the queryClient.setQueryData([QueryKey.RepoStatus, owner, repoName], conflictStatus) write; then replace the inline block in VersionControlButtonsContext (the if branch that currently constructs conflictStatus and calls queryClient.setQueryData) with a single call to that helper and reuse the same helper from FetchChangesPopover so both flows share the identical status literals and cache-write behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx`:
- Around line 59-66: The refetch result from fetchPullData() can be undefined
causing result.hasMergeConflict to throw and isLoading to remain true; update
the logic in VersionControlButtonsContext so you first check that the
QueryObserverResult data exists (e.g., if (!result) { setIsLoading(false);
return; }) before accessing result.hasMergeConflict or result.repositoryStatus,
and always clear the loading state (setIsLoading(false)) in the non-conflict and
error paths (or a finally-equivalent) so isLoading cannot get stuck; ensure you
still construct conflictStatus and call
queryClient.setQueryData([QueryKey.RepoStatus, owner, repoName], conflictStatus)
only when result is defined and indicates a conflict.
---
Nitpick comments:
In
`@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs`:
- Around line 1524-1689: The TryRecoverPullCheckoutConflictWithAutoStash method
repeats the same rollback-and-return logic in several places (on pull conflict,
apply conflict, and in the exception handler) — extract that into a small helper
that accepts the repo, headCommitBeforePull, authenticatedContext, stashIndex, a
rollbackReason string, a flag indicating whether applyStarted, the current
mergeStatus and mergeConflictCount, and the Activity so it can call
SetErrorStatus, invoke RollbackAndRestoreAutoStash, throw an
InvalidOperationException if rollback fails, and return the appropriate
AutoStashRecoveryResult; then replace each duplicated block in
TryRecoverPullCheckoutConflictWithAutoStash with calls to this new helper to
reduce duplication and centralize error/rollback handling.
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx`:
- Around line 60-66: Extract the duplicate conflict-state update into a small
shared helper (e.g., updateConflictStatus or setRepoConflictStatus) that takes
the queryClient, owner, repoName and the original result (RepoStatus) and
performs the hasMergeConflict merge plus the
queryClient.setQueryData([QueryKey.RepoStatus, owner, repoName], conflictStatus)
write; then replace the inline block in VersionControlButtonsContext (the if
branch that currently constructs conflictStatus and calls
queryClient.setQueryData) with a single call to that helper and reuse the same
helper from FetchChangesPopover so both flows share the identical status
literals and cache-write behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50d409a4-1f54-40ab-af55-1365be84e386
📒 Files selected for processing (7)
src/Designer/backend/src/Designer/Controllers/RepositoryController.cssrc/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cssrc/Designer/backend/src/Designer/Services/Interfaces/ISourceControl.cssrc/Designer/backend/tests/Designer.Tests/Controllers/RepositoryController/CommitAndPushRepoTests.cssrc/Designer/backend/tests/Designer.Tests/Mocks/ISourceControlMock.cssrc/Designer/backend/tests/Designer.Tests/Services/SourceControlServiceTest.cssrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx
martinothamar
force-pushed
the
fix/pull-conflict-issue
branch
2 times, most recently
from
224d815 to
837b9fb
Compare
martinothamar
force-pushed
the
fix/pull-conflict-issue
branch
from
2910e52 to
929bfe9
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.test.tsx (1)
246-248: UsegetByRole('alert')instead ofgetByText()for the toast assertion.Line 247 currently uses
screen.getByText(...)for a toast element. Since the test renders withServicesContextProvider(which includesToastContainer), toast elements have thealertrole and should be queried semantically per your testing guidelines.Suggested refactor
- await waitFor(() => { - expect( - screen.getByText(textMock('sync_header.checkout_conflict_blocked_action')), - ).toBeInTheDocument(); - }); + await waitFor(() => { + expect(screen.getByRole('alert')).toHaveTextContent( + textMock('sync_header.checkout_conflict_blocked_action'), + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.test.tsx` around lines 246 - 248, The test is querying the toast text with screen.getByText(...) which is non-semantic for toasts rendered by ToastContainer; update the assertion in VersionControlButtonsContext.test.tsx to use screen.getByRole('alert') to find the toast element and then assert that its text content matches textMock('sync_header.checkout_conflict_blocked_action') (i.e., replace the getByText call with getByRole('alert') and check its text content), leaving the surrounding test scaffolding (ServicesContextProvider, ToastContainer) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.test.tsx`:
- Around line 246-248: The test is querying the toast text with
screen.getByText(...) which is non-semantic for toasts rendered by
ToastContainer; update the assertion in VersionControlButtonsContext.test.tsx to
use screen.getByRole('alert') to find the toast element and then assert that its
text content matches textMock('sync_header.checkout_conflict_blocked_action')
(i.e., replace the getByText call with getByRole('alert') and check its text
content), leaving the surrounding test scaffolding (ServicesContextProvider,
ToastContainer) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7001b7c-88f6-4d8c-8dd3-60314e1558c7
📒 Files selected for processing (9)
src/Designer/frontend/language/src/en.jsonsrc/Designer/frontend/language/src/nb.jsonsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.test.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.test.tssrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.tssrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/utils/repoStatus.ts
✅ Files skipped from review due to trivial changes (3)
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.test.ts
- src/Designer/frontend/language/src/en.json
- src/Designer/frontend/language/src/nb.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.ts
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsx
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/utils/repoStatus.ts
Contributor
Author
|
This was a PITA, but did rework this to try to simplify and have tested a lot now:
One UX-related issue remain: in case nr 2 above where there is a conflict I currently get an error toast right before getting prompted the conflict screen EDIT: now also tested the UX issue, dont get the error toast when being redirected to conflict view anymore |
martinothamar
force-pushed
the
fix/pull-conflict-issue
branch
from
cb39117 to
bbfad2b
Compare
martinothamar
force-pushed
the
fix/pull-conflict-issue
branch
from
bbfad2b to
10f3243
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx (1)
83-86:⚠️ Potential issue | 🟠 MajorGuard against undefined
resulton the success path.After a successful push,
fetchPullData()is called andresult.repositoryStatusis accessed without checking ifresultis defined. If the refetch fails or returns no cached data, this will throw.Proposed fix
const { data: result } = await fetchPullData(); - if (result.repositoryStatus === 'Ok') { + if (result?.repositoryStatus === 'Ok') { toast.success(t('sync_header.sharing_changes_completed')); } + setIsLoading(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx` around lines 83 - 86, After calling fetchPullData() in VersionControlButtonsContext, guard against result being undefined before accessing result.repositoryStatus: update the success-path check around the fetch in the function that calls fetchPullData() so it validates result (e.g., result && result.repositoryStatus === 'Ok' or use optional chaining result?.repositoryStatus) before calling toast.success(t('sync_header.sharing_changes_completed')); leave behavior unchanged when result is Ok and simply skip the toast when result is undefined or a non-Ok status.
♻️ Duplicate comments (2)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx (1)
71-80:⚠️ Potential issue | 🟠 MajorGuard against undefined
resultfromfetchPullData().Per TanStack Query v5,
refetch()returns aQueryObserverResultwheredatacan beundefinedwhen there's no cached data or the refetch fails. Destructuring as{ data: result }leavesresultundefined, and subsequent access tohasRepoMergeConflict(result)orhasCheckoutConflict(result)may not handle this correctly.Additionally, if neither condition matches (lines 74-79),
setIsLoading(false)is never called, leaving the loading state stuck.Proposed fix
} catch (error) { console.error(error); - const { data: result } = await fetchPullData(); - if (hasRepoMergeConflict(result)) { + const pullResult = await fetchPullData().catch(() => ({ data: undefined })); + const result = pullResult?.data; + if (!result) { + setIsLoading(false); + return; + } + if (hasRepoMergeConflict(result)) { setMergeConflictState(result); } else if (hasCheckoutConflict(result)) { toast.error(t('sync_header.checkout_conflict_blocked_action')); setIsLoading(false); + } else { + setIsLoading(false); } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx` around lines 71 - 80, The catch block must guard against an undefined result from fetchPullData() and ensure loading is cleared in all branches: after awaiting fetchPullData() (the refetch helper used here) check whether the destructured data exists before calling hasRepoMergeConflict(result) or hasCheckoutConflict(result); if result is undefined, call setIsLoading(false) (and optionally log) and return. Keep the existing branches that call setMergeConflictState(result) and toast.error(...) but ensure every code path — repo merge conflict, checkout conflict, and the fallback when neither condition matches or data is undefined — ends with setIsLoading(false) so the loading state is never left stuck.src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs (1)
996-1003:⚠️ Potential issue | 🟠 MajorKeep the stop path on the non-fast-forward exception contract.
Throwing
InvalidOperationExceptionhere changes the error type emitted by the recovery flow. If a remote rebase stops, this can bypass the normal conflict handling and surface as a 500 instead of the expected conflict response.Suggested fix
- throw new InvalidOperationException( - $"Rebase onto remote branch '{ctx.branchName}' was stopped by user." - ); + throw new NonFastForwardException( + $"Rebase onto remote branch '{ctx.branchName}' was stopped by user." + );#!/bin/bash # Verify that the remote-rebase recovery path is still handled as a NonFastForwardException # by the controller/filter layer. rg -n -C2 'RebaseStatus\.Stop|throw new (InvalidOperationException|NonFastForwardException)' \ src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs echo rg -n -C3 'RebaseOntoRemoteBranch|NonFastForwardException|InvalidOperationException' \ src/Designer/backend/src/Designer/Controllers/RepositoryController.cs \ src/Designer/backend/src/Designer/Filters/Git/GitExceptionFilterAttribute.cs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs` around lines 996 - 1003, The RebaseStatus.Stop branch currently throws InvalidOperationException which breaks the expected NonFastForwardException contract used by the recovery/filter layer; change the throw in the RebaseStatus.Stop path to throw the NonFastForwardException used elsewhere (preserving repo.Rebase.Abort(), stopAborted and SetErrorStatus(ctx.activity, "rebase_stopped")), and construct the NonFastForwardException with the same message/context (including ctx.branchName) so the controller/ GitExceptionFilterAttribute can handle it as a non-fast-forward/conflict case.
🧹 Nitpick comments (4)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx (1)
10-16: Consider removing orphanedforceRepoStatusChecklisteners.The listeners in
useListenToMergeConflictInRepo.tsandPageLayout.tsxlisten forforceRepoStatusCheckmessages, but no code in the Designer frontend sends this message. These listeners are orphaned and should be removed or refactored as a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx` around lines 10 - 16, Listeners for the 'forceRepoStatusCheck' message in useListenToMergeConflictInRepo and PageLayout are orphaned (no sender exists); remove the event listener registration and any related cleanup code in those modules (e.g., the addEventListener/on listener that listens for 'forceRepoStatusCheck' and its corresponding removeEventListener/cleanup) or refactor to a shared, documented mechanism if you intend to keep the feature; ensure you also delete any references to the handler functions so no unused symbols remain.src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs (1)
945-947: Reuse the open repository for the fetch.This method fetches through the public wrapper and then immediately opens the repository again. Calling the private overload on the same
repoinstance would avoid the extra repo handle, nested telemetry, and one redundant fetch in the pull→rebase path.Possible tidy-up
- self.FetchRemoteChanges(ctx.authenticatedContext); - using LibGit2Sharp.Repository repo = self.CreateLocalRepo(ctx.authenticatedContext); + using LibGit2Sharp.Repository repo = self.CreateLocalRepo(ctx.authenticatedContext); + self.FetchRemoteChanges(repo, ctx.authenticatedContext); self.FetchGitNotes(repo, ctx.authenticatedContext);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs` around lines 945 - 947, The code currently calls the public wrapper FetchRemoteChanges(ctx.authenticatedContext) then opens the repo with CreateLocalRepo and calls FetchGitNotes(repo,...), causing a second repo open and redundant fetch; instead, remove the initial public call and call the private overload that takes the opened LibGit2Sharp.Repository (e.g. FetchRemoteChanges(repo, ctx.authenticatedContext)) after CreateLocalRepo so the same repo instance is reused for fetching and notes, avoiding the extra handle, nested telemetry and duplicate fetch.src/Designer/backend/tests/Designer.Tests/Utils/GitRepositoryTestHelper.cs (1)
50-77: Promote the studio-note protocol strings to shared constants.This helper now hard-codes the note message and ref names that production code also relies on. Centralising them will make future note-format changes much safer across service code and tests.
As per coding guidelines, "Avoid hard-coded numbers and strings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/backend/tests/Designer.Tests/Utils/GitRepositoryTestHelper.cs` around lines 50 - 77, The test helper hard-codes note protocol strings ("refs/notes/*:refs/notes/*", "studio-commit", "refs/notes/commits") inside the code paths around Commands.Fetch, repo.Notes.Add, and repo.Network.Push; extract these literals into shared constants (e.g., NoteRefSpec, StudioNoteMessage, NotesPushRef or a NotesConstants class/enum) used by both production code and this test helper, update the calls in GitRepositoryTestHelper (the Fetch call, repo.Notes.Add call, and the repo.Network.Push call) to reference those constants, and update any imports/usings so tests and production code reference the same source of truth.src/Designer/backend/tests/Designer.Tests/Controllers/RepositoryController/CommitAndPushRepoTests.cs (1)
28-40: Prefer aMoqregistration forIGiteaClient.Everything else in this fixture is driven by
Mock<>, so the concrete singleton fake makes per-test behaviour less explicit and harder to tailor as this class grows.As per coding guidelines "Mock external dependencies using `Moq`".♻️ Suggested change
private readonly Mock<ISourceControl> _sourceControlMock = new(); + private readonly Mock<IGiteaClient> _giteaClientMock = new(); private readonly Mock<IHubContext<SyncHub, ISyncClient>> _syncHubContextMock = new();- services.AddSingleton<IGiteaClient, IGiteaClientMock>(); + services.AddSingleton(_giteaClientMock.Object);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/backend/tests/Designer.Tests/Controllers/RepositoryController/CommitAndPushRepoTests.cs` around lines 28 - 40, Replace the concrete IGiteaClientMock singleton with a Moq-backed registration so tests can customize behavior per test: add a private readonly Mock<IGiteaClient> (e.g. _giteaClientMock) alongside the other Mock<> fields and, in ConfigureTestServices, register its Object with the DI container instead of IGiteaClientMock (services.AddSingleton<IGiteaClient>(_giteaClientMock.Object) or equivalent). Update any test setups to configure _giteaClientMock.Setup(...) as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Designer/backend/tests/Designer.Tests/Services/SourceControlServiceTest.cs`:
- Around line 413-451: The test currently asserts
DesignerRepositoryStatus.CheckoutConflict after calling PullRemoteChanges, but
given the PR's stash→rebase→stash-pop flow you should assert the success path
instead: update the assertion on the RepoStatus returned from
Service.PullRemoteChanges (and remove the hard expectation of
DesignerRepositoryStatus.CheckoutConflict) to verify the operation completed
successfully (i.e. not CheckoutConflict), then assert the repository reflects a
successful rebase/pull — the local HEAD should advance to include the remote
commit, remote-only.txt should exist, local-only.txt should still exist and the
working tree remains dirty, and localRepo.Index.Conflicts remains empty; use the
PullRemoteChanges method, the test fixture created by
CreateTrackedRepositoryForPull, and DesignerRepositoryStatus.CheckoutConflict as
reference points to find and change the assertions.
---
Outside diff comments:
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx`:
- Around line 83-86: After calling fetchPullData() in
VersionControlButtonsContext, guard against result being undefined before
accessing result.repositoryStatus: update the success-path check around the
fetch in the function that calls fetchPullData() so it validates result (e.g.,
result && result.repositoryStatus === 'Ok' or use optional chaining
result?.repositoryStatus) before calling
toast.success(t('sync_header.sharing_changes_completed')); leave behavior
unchanged when result is Ok and simply skip the toast when result is undefined
or a non-Ok status.
---
Duplicate comments:
In
`@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs`:
- Around line 996-1003: The RebaseStatus.Stop branch currently throws
InvalidOperationException which breaks the expected NonFastForwardException
contract used by the recovery/filter layer; change the throw in the
RebaseStatus.Stop path to throw the NonFastForwardException used elsewhere
(preserving repo.Rebase.Abort(), stopAborted and SetErrorStatus(ctx.activity,
"rebase_stopped")), and construct the NonFastForwardException with the same
message/context (including ctx.branchName) so the controller/
GitExceptionFilterAttribute can handle it as a non-fast-forward/conflict case.
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx`:
- Around line 71-80: The catch block must guard against an undefined result from
fetchPullData() and ensure loading is cleared in all branches: after awaiting
fetchPullData() (the refetch helper used here) check whether the destructured
data exists before calling hasRepoMergeConflict(result) or
hasCheckoutConflict(result); if result is undefined, call setIsLoading(false)
(and optionally log) and return. Keep the existing branches that call
setMergeConflictState(result) and toast.error(...) but ensure every code path —
repo merge conflict, checkout conflict, and the fallback when neither condition
matches or data is undefined — ends with setIsLoading(false) so the loading
state is never left stuck.
---
Nitpick comments:
In
`@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs`:
- Around line 945-947: The code currently calls the public wrapper
FetchRemoteChanges(ctx.authenticatedContext) then opens the repo with
CreateLocalRepo and calls FetchGitNotes(repo,...), causing a second repo open
and redundant fetch; instead, remove the initial public call and call the
private overload that takes the opened LibGit2Sharp.Repository (e.g.
FetchRemoteChanges(repo, ctx.authenticatedContext)) after CreateLocalRepo so the
same repo instance is reused for fetching and notes, avoiding the extra handle,
nested telemetry and duplicate fetch.
In
`@src/Designer/backend/tests/Designer.Tests/Controllers/RepositoryController/CommitAndPushRepoTests.cs`:
- Around line 28-40: Replace the concrete IGiteaClientMock singleton with a
Moq-backed registration so tests can customize behavior per test: add a private
readonly Mock<IGiteaClient> (e.g. _giteaClientMock) alongside the other Mock<>
fields and, in ConfigureTestServices, register its Object with the DI container
instead of IGiteaClientMock
(services.AddSingleton<IGiteaClient>(_giteaClientMock.Object) or equivalent).
Update any test setups to configure _giteaClientMock.Setup(...) as needed.
In `@src/Designer/backend/tests/Designer.Tests/Utils/GitRepositoryTestHelper.cs`:
- Around line 50-77: The test helper hard-codes note protocol strings
("refs/notes/*:refs/notes/*", "studio-commit", "refs/notes/commits") inside the
code paths around Commands.Fetch, repo.Notes.Add, and repo.Network.Push; extract
these literals into shared constants (e.g., NoteRefSpec, StudioNoteMessage,
NotesPushRef or a NotesConstants class/enum) used by both production code and
this test helper, update the calls in GitRepositoryTestHelper (the Fetch call,
repo.Notes.Add call, and the repo.Network.Push call) to reference those
constants, and update any imports/usings so tests and production code reference
the same source of truth.
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx`:
- Around line 10-16: Listeners for the 'forceRepoStatusCheck' message in
useListenToMergeConflictInRepo and PageLayout are orphaned (no sender exists);
remove the event listener registration and any related cleanup code in those
modules (e.g., the addEventListener/on listener that listens for
'forceRepoStatusCheck' and its corresponding removeEventListener/cleanup) or
refactor to a shared, documented mechanism if you intend to keep the feature;
ensure you also delete any references to the handler functions so no unused
symbols remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 470ef8ea-f6cc-4c7b-b686-33f8c31c8c97
📒 Files selected for processing (24)
src/Designer/backend/src/Designer/Controllers/RepositoryController.cssrc/Designer/backend/src/Designer/Models/RemoteRebaseResult.cssrc/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cssrc/Designer/backend/src/Designer/Services/Interfaces/ISourceControl.cssrc/Designer/backend/tests/Designer.Tests/Controllers/RepositoryController/CommitAndPushRepoTests.cssrc/Designer/backend/tests/Designer.Tests/GiteaIntegrationTests/RepositoryController/GitNotesGiteaIntegrationTests.cssrc/Designer/backend/tests/Designer.Tests/GiteaIntegrationTests/RepositoryController/RepositoryControllerGiteaIntegrationTests.cssrc/Designer/backend/tests/Designer.Tests/Mocks/ISourceControlMock.cssrc/Designer/backend/tests/Designer.Tests/Services/SourceControlServiceTest.cssrc/Designer/backend/tests/Designer.Tests/StudioOidcGiteaIntegrationTests/RepositoryController/GitNotesStudioOidcTests.cssrc/Designer/backend/tests/Designer.Tests/StudioOidcGiteaIntegrationTests/RepositoryController/RepositoryControllerStudioOidcTests.cssrc/Designer/backend/tests/Designer.Tests/Utils/GitRepositoryTestHelper.cssrc/Designer/frontend/language/src/en.jsonsrc/Designer/frontend/language/src/nb.jsonsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/ShareChangesPopover/ShareChangesPopover.test.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/ShareChangesPopover/ShareChangesPopover.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.test.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.test.tssrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.tssrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/utils/repoStatus.tssrc/Designer/frontend/packages/shared/src/hooks/mutations/useRepoCommitAndPushMutation.ts
✅ Files skipped from review due to trivial changes (3)
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.test.ts
- src/Designer/frontend/language/src/en.json
- src/Designer/frontend/language/src/nb.json
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Designer/backend/tests/Designer.Tests/Mocks/ISourceControlMock.cs
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.ts
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsx
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.tsx
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/utils/repoStatus.ts
- src/Designer/backend/src/Designer/Controllers/RepositoryController.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx (1)
83-86:⚠️ Potential issue | 🟠 MajorAdd optional chaining and reset loading state in success path.
On line 84,
result.repositoryStatusis accessed without checking ifresultis defined. SincefetchPullData()returnsUseQueryResult<RepoStatus>wheredatacan beundefined, this will throw a runtime error if the refetch completes without data.Additionally,
setIsLoading(false)is missing from the success path. The loading state is set totrueat the start ofcommitAndPushChanges(line 65) but only cleared in error branches (line 78 and viasetMergeConflictState), leaving it stuck when the operation succeeds.Proposed fix
const { data: result } = await fetchPullData(); - if (result.repositoryStatus === 'Ok') { + if (result?.repositoryStatus === 'Ok') { toast.success(t('sync_header.sharing_changes_completed')); } + setIsLoading(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx` around lines 83 - 86, The success path in commitAndPushChanges uses the refetch result unsafely and doesn't clear loading: after calling fetchPullData() extract data with optional chaining (e.g., check result?.repositoryStatus) before comparing to 'Ok' and call setIsLoading(false) in the success branch so the loading state is always cleared; update the block handling the fetchPullData() result in commitAndPushChanges to use optional chaining on result.repositoryStatus and invoke setIsLoading(false) before showing toast.success (and ensure other early returns still clear loading or rely on a single finally-style clear).
♻️ Duplicate comments (3)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx (1)
71-80:⚠️ Potential issue | 🟠 MajorLoading state not cleared when error occurs without conflict detection.
If
repoCommitAndPushMutationfails andfetchPullData()returns a result that is neither a merge conflict nor a checkout conflict (e.g., an unexpected state orresultbeing undefined), the code reachesreturnon line 80 without ever callingsetIsLoading(false). This leaves the UI stuck in a loading state.Proposed fix
} catch (error) { console.error(error); - const { data: result } = await fetchPullData(); + const pullResult = await fetchPullData().catch(() => undefined); + const result = pullResult?.data; if (hasRepoMergeConflict(result)) { setMergeConflictState(result); } else if (hasCheckoutConflict(result)) { toast.error(t('sync_header.checkout_conflict_blocked_action')); setIsLoading(false); + } else { + setIsLoading(false); } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx` around lines 71 - 80, The catch block currently logs the error, calls fetchPullData(), and returns without clearing the loading flag if the fetched result is neither a merge nor checkout conflict; update the catch handling so that after calling fetchPullData() and evaluating hasRepoMergeConflict(result) and hasCheckoutConflict(result) you call setIsLoading(false) before returning in the non-conflict path (and also handle a possibly undefined result safely), keeping existing behavior for setMergeConflictState(...) and toast.error(...); references: repoCommitAndPushMutation catch block, fetchPullData(), hasRepoMergeConflict, hasCheckoutConflict, setMergeConflictState, toast.error, setIsLoading.src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs (2)
154-162:⚠️ Potential issue | 🟠 MajorDon't block every dirty diverged pull.
This now returns
CheckoutConflictfor any dirty worktree once the branch is both ahead and behind, sopullstill fails in the “remote changed file B, I have uncommitted file C” case that the PR description/manual verification say should succeed. This needs the stash → rebase/pull → stash-pop flow (or an equivalent overlap check) and should only fall back toCheckoutConflictwhen replaying the dirty changes actually fails. The newPullRemoteChanges_DirtyAndDiverged_ReturnsCheckoutConflictWithoutRebasingtest is currently pinning the regressed behaviour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs` around lines 154 - 162, The current logic in the block where behindBy > 0 && aheadBy > 0 unconditionally treats any dirty worktree (isDirtyBefore) as a CheckoutConflict by setting status.RepositoryStatus = Enums.RepositoryStatus.CheckoutConflict and returning; instead, update the flow in the method that contains this logic (the PullRemoteChanges path) to perform the stash → pull/rebase → stash-pop sequence (or equivalent replay) when isDirtyBefore is true and the branch is diverged (behindBy > 0 && aheadBy > 0), only marking CheckoutConflict and calling SetErrorStatus(ctx.activity, "checkout_conflict") if applying the stashed changes fails (i.e., stash-pop/replay reports conflicts); preserve checkoutConflict variable handling and only return early on a real replay failure.
996-1003:⚠️ Potential issue | 🟠 MajorThrow
NonFastForwardExceptionfrom the stopped-rebase path.
InvalidOperationExceptionescapes the controller/filter conflict handling, so a stopped remote rebase becomes a 500 instead of the expected 409 conflict response.Suggested fix
- throw new InvalidOperationException( - $"Rebase onto remote branch '{ctx.branchName}' was stopped by user." - ); + throw new NonFastForwardException( + $"Rebase onto remote branch '{ctx.branchName}' was stopped by user." + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs` around lines 996 - 1003, The stopped-rebase branch currently throws InvalidOperationException which bypasses the controller's conflict handling; in the block that checks if (rebaseResult.Status == RebaseStatus.Stop) (where repo.Rebase.Abort(), stopAborted = true, and SetErrorStatus(ctx.activity, "rebase_stopped") are called), replace the thrown InvalidOperationException with throwing NonFastForwardException (preserving the same message referencing ctx.branchName) so the controller/filter treats the stopped rebase as a 409 conflict; ensure the exception type is imported/available where SourceControlService's rebase handling code runs.
🧹 Nitpick comments (1)
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/ShareChangesPopover/ShareChangesPopover.test.tsx (1)
219-226: UsefindByRoleinstead ofwaitFor+getByRolefor async queries.The static analysis correctly flags this.
findByRoleis the idiomatic RTL pattern for waiting on elements to appear and is more concise.Proposed fix
- await waitFor(() => - expect( - screen.getByRole('heading', { - level: 3, - name: textMock('sync_header.describe_and_validate'), - }), - ).toBeInTheDocument(), - ); + expect( + await screen.findByRole('heading', { + level: 3, + name: textMock('sync_header.describe_and_validate'), + }), + ).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/ShareChangesPopover/ShareChangesPopover.test.tsx` around lines 219 - 226, Replace the waitFor + getByRole pattern in the test with the async findByRole call: await screen.findByRole('heading', { level: 3, name: textMock('sync_header.describe_and_validate') }); specifically update the assertion in ShareChangesPopover.test.tsx (the block using waitFor(...) expect(screen.getByRole(...)).toBeInTheDocument()) to use await screen.findByRole(...) and assert its presence, removing the surrounding waitFor wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Designer/backend/src/Designer/Controllers/RepositoryController.cs`:
- Around line 398-399: The call to NotifyFileSyncSuccesses (after
_sourceControl.PublishBranch) should be treated as best-effort so failures don't
convert a successful publish into a 500; wrap the NotifyFileSyncSuccesses
invocation(s) (the occurrence here and the similar block at the other occurrence
around lines 403-412) in a try/catch, log the exception (using the controller's
logger or existing logging mechanism) and do not rethrow, allowing the method to
return success after PublishBranch completes; keep PublishBranch calls unchanged
and only catch/log notification/send errors from NotifyFileSyncSuccesses/SignalR
sends.
---
Outside diff comments:
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx`:
- Around line 83-86: The success path in commitAndPushChanges uses the refetch
result unsafely and doesn't clear loading: after calling fetchPullData() extract
data with optional chaining (e.g., check result?.repositoryStatus) before
comparing to 'Ok' and call setIsLoading(false) in the success branch so the
loading state is always cleared; update the block handling the fetchPullData()
result in commitAndPushChanges to use optional chaining on
result.repositoryStatus and invoke setIsLoading(false) before showing
toast.success (and ensure other early returns still clear loading or rely on a
single finally-style clear).
---
Duplicate comments:
In
`@src/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cs`:
- Around line 154-162: The current logic in the block where behindBy > 0 &&
aheadBy > 0 unconditionally treats any dirty worktree (isDirtyBefore) as a
CheckoutConflict by setting status.RepositoryStatus =
Enums.RepositoryStatus.CheckoutConflict and returning; instead, update the flow
in the method that contains this logic (the PullRemoteChanges path) to perform
the stash → pull/rebase → stash-pop sequence (or equivalent replay) when
isDirtyBefore is true and the branch is diverged (behindBy > 0 && aheadBy > 0),
only marking CheckoutConflict and calling SetErrorStatus(ctx.activity,
"checkout_conflict") if applying the stashed changes fails (i.e.,
stash-pop/replay reports conflicts); preserve checkoutConflict variable handling
and only return early on a real replay failure.
- Around line 996-1003: The stopped-rebase branch currently throws
InvalidOperationException which bypasses the controller's conflict handling; in
the block that checks if (rebaseResult.Status == RebaseStatus.Stop) (where
repo.Rebase.Abort(), stopAborted = true, and SetErrorStatus(ctx.activity,
"rebase_stopped") are called), replace the thrown InvalidOperationException with
throwing NonFastForwardException (preserving the same message referencing
ctx.branchName) so the controller/filter treats the stopped rebase as a 409
conflict; ensure the exception type is imported/available where
SourceControlService's rebase handling code runs.
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsx`:
- Around line 71-80: The catch block currently logs the error, calls
fetchPullData(), and returns without clearing the loading flag if the fetched
result is neither a merge nor checkout conflict; update the catch handling so
that after calling fetchPullData() and evaluating hasRepoMergeConflict(result)
and hasCheckoutConflict(result) you call setIsLoading(false) before returning in
the non-conflict path (and also handle a possibly undefined result safely),
keeping existing behavior for setMergeConflictState(...) and toast.error(...);
references: repoCommitAndPushMutation catch block, fetchPullData(),
hasRepoMergeConflict, hasCheckoutConflict, setMergeConflictState, toast.error,
setIsLoading.
---
Nitpick comments:
In
`@src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/ShareChangesPopover/ShareChangesPopover.test.tsx`:
- Around line 219-226: Replace the waitFor + getByRole pattern in the test with
the async findByRole call: await screen.findByRole('heading', { level: 3, name:
textMock('sync_header.describe_and_validate') }); specifically update the
assertion in ShareChangesPopover.test.tsx (the block using waitFor(...)
expect(screen.getByRole(...)).toBeInTheDocument()) to use await
screen.findByRole(...) and assert its presence, removing the surrounding waitFor
wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cdeb91f-6771-4352-874d-ac9f33328da3
📒 Files selected for processing (24)
src/Designer/backend/src/Designer/Controllers/RepositoryController.cssrc/Designer/backend/src/Designer/Models/RemoteRebaseResult.cssrc/Designer/backend/src/Designer/Services/Implementation/SourceControlService.cssrc/Designer/backend/src/Designer/Services/Interfaces/ISourceControl.cssrc/Designer/backend/tests/Designer.Tests/Controllers/RepositoryController/CommitAndPushRepoTests.cssrc/Designer/backend/tests/Designer.Tests/GiteaIntegrationTests/RepositoryController/GitNotesGiteaIntegrationTests.cssrc/Designer/backend/tests/Designer.Tests/GiteaIntegrationTests/RepositoryController/RepositoryControllerGiteaIntegrationTests.cssrc/Designer/backend/tests/Designer.Tests/Mocks/ISourceControlMock.cssrc/Designer/backend/tests/Designer.Tests/Services/SourceControlServiceTest.cssrc/Designer/backend/tests/Designer.Tests/StudioOidcGiteaIntegrationTests/RepositoryController/GitNotesStudioOidcTests.cssrc/Designer/backend/tests/Designer.Tests/StudioOidcGiteaIntegrationTests/RepositoryController/RepositoryControllerStudioOidcTests.cssrc/Designer/backend/tests/Designer.Tests/Utils/GitRepositoryTestHelper.cssrc/Designer/frontend/language/src/en.jsonsrc/Designer/frontend/language/src/nb.jsonsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/ShareChangesPopover/ShareChangesPopover.test.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/ShareChangesPopover/ShareChangesPopover.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.test.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.tsxsrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.test.tssrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.tssrc/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/utils/repoStatus.tssrc/Designer/frontend/packages/shared/src/hooks/mutations/useRepoCommitAndPushMutation.ts
✅ Files skipped from review due to trivial changes (8)
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.test.ts
- src/Designer/frontend/language/src/en.json
- src/Designer/backend/tests/Designer.Tests/GiteaIntegrationTests/RepositoryController/RepositoryControllerGiteaIntegrationTests.cs
- src/Designer/frontend/language/src/nb.json
- src/Designer/backend/src/Designer/Models/RemoteRebaseResult.cs
- src/Designer/backend/tests/Designer.Tests/GiteaIntegrationTests/RepositoryController/GitNotesGiteaIntegrationTests.cs
- src/Designer/backend/tests/Designer.Tests/Controllers/RepositoryController/CommitAndPushRepoTests.cs
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/utils/repoStatus.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/ShareChangesPopover/ShareChangesPopover.tsx
- src/Designer/backend/tests/Designer.Tests/Mocks/ISourceControlMock.cs
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.test.tsx
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/FetchChangesPopover/FetchChangesPopover.tsx
- src/Designer/frontend/packages/shared/src/hooks/mutations/useRepoCommitAndPushMutation.ts
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/hooks/useHasMergeConflict/useHasMergeConflict.ts
- src/Designer/backend/tests/Designer.Tests/StudioOidcGiteaIntegrationTests/RepositoryController/RepositoryControllerStudioOidcTests.cs
- src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/context/VersionControlButtonsContext/VersionControlButtonsContext.test.tsx
- src/Designer/backend/tests/Designer.Tests/Utils/GitRepositoryTestHelper.cs
martinothamar
force-pushed
the
fix/pull-conflict-issue
branch
from
10f3243 to
9c7c7a0
Compare
mlqn
approved these changes
Mar 20, 2026
Contributor
There was a problem hiding this comment.
Nice work 🤩 I tested locally and I can confirm the bug is fixed 💪
I noticed just one odd behavior while testing, but it doesn’t seem critical and can probably be fixed later in a separate PR:
If I make some changes to the same file in studio and in gitea, and then click:
- Hent endringer: it shows a toast error
- Del dine endringer: it shows the conflict dialog
But if I refresh the browser, and click:
- Hent endringer: it shows the conflict dialog
- Del dine endringer: it then no longer shows my changes, which is a bit unexpected
changes.mov
| import { type RepoStatus } from 'app-shared/types/RepoStatus'; | ||
|
|
||
| export const hasRepoMergeConflict = (repoStatus?: Partial<RepoStatus>): boolean => | ||
| Boolean(repoStatus?.hasMergeConflict || repoStatus?.repositoryStatus === 'MergeConflict'); |
Contributor
There was a problem hiding this comment.
Just a small note (not specific to this PR, since this logic already existed before). It looks like we check two properties from the backend to detect a conflict, and then adjust the response on the client with toMergeConflictRepoStatus. This could probably be simplified if the backend returned a single property for a conflict status, but I’m not familiar with this part of the backend code and I know there may be limitations with LibGit2Sharp, so just sharing this as a general observation
… commit-and-push non-conflict path (different files)
martinothamar
force-pushed
the
fix/pull-conflict-issue
branch
from
9c7c7a0 to
4077b6a
Compare
Contributor
Author
|
@mlqn right, when we try to rebase now as part of "Del dine endringer", the changes have already been committed. I guess we can update the frontend to account for commited but not pushed changes? Or undo the local unpushed commit on the conflict case? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix for #17499
There are a couple of scenarios where we have issues today, essentially getting merge commits (and empty commit message in one instance):
Merge commit on Push ("Del endringer")
Results in backend retry path where a merge commit is produced (no conflicts). This is perhaps not a bug but arguably bad UX
Merge commit on Pull ("Hent endringer")
Results in a merge commit containing both R1 and a commit with empty message (the one from
commitAndPusChanges('')). This matches the issue reported, and it is very unexpected to have commits appearing from pulling (it doesnt sound mutating)With this PR
NonFastForwardExceptionMerge commit from second scenario is a little more complicated, but here we try to essentially. Currently have a simpler variant where we try to rebase on the remote ref only if there are no changes to the same file. Just to be a bit conservative and make things less complicated. If same file changes when doing pull we get an error toast and ask the user to "Del endringer" instead which will end up comitting and trying to rebase that commit on top of the remote ref instead.stash -u,pull,stash popto see if we can get by that way (if it fails we try to revert to original state, see code comment). Any conflict should be handled by going to the conflict state in frontend where users are presented with the choice of downloading repo/changes and reverting uncomitted work, so this is still best effortVerification
Summary by CodeRabbit
Bug Fixes
New Features
UI
Tests