feat: Merge conflict resolution#561
Merged
NagariaHussain merged 4 commits intodevelopfrom Feb 20, 2026
Merged
Conversation
WalkthroughThe change adds an interactive per-conflict merge workflow. Frontend updates replace the single Merge action with conflict-aware UI, per-conflict expansion, diff viewers, and Keep Main/Keep Your Changes controls; new state and resources handle conflict loading, resolutions, and retrying merges. Backend adds three public functions—get_merge_conflicts, resolve_merge_conflict, and retry_merge_after_resolution—and an end-to-end test that exercises conflict detection, resolution (using "theirs"), and a subsequent merge retry. No existing public signatures were removed. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
wiki/frappe_wiki/doctype/wiki_change_request/wiki_change_request.py (1)
827-849: Extra DB query per resolved conflict to re-derive content_blob.When
resolved.get("content_blob")is falsy (Lines 833-845), the code queriesWiki Merge Conflictagain for each such key to determine the resolution side. This information is already available inresolved_conflicts(fetched at Line 791). Consider enrichingresolved_mapwith theresolutionfield upfront to avoid the extra queries.Proposed optimization
resolved_conflicts = frappe.get_all( "Wiki Merge Conflict", filters={"change_request": name, "status": "Resolved"}, - fields=["doc_key", "resolved_payload"], + fields=["doc_key", "resolved_payload", "resolution"], ) - resolved_map: dict[str, dict[str, Any]] = {} + resolved_map: dict[str, tuple[dict[str, Any], str]] = {} for rc in resolved_conflicts: payload = rc.get("resolved_payload") if isinstance(payload, str): payload = frappe.parse_json(payload) - resolved_map[rc["doc_key"]] = payload + resolved_map[rc["doc_key"]] = (payload, rc.get("resolution", ""))Then update the usage at Line 828+ to unpack the tuple and use the cached resolution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/frappe_wiki/doctype/wiki_change_request/wiki_change_request.py` around lines 827 - 849, The loop is re-querying "Wiki Merge Conflict" for each resolved key to find the resolution side; instead, when building resolved_map from resolved_conflicts (the data fetched into resolved_conflicts), include the resolution field in each resolved_map entry (e.g., store a dict with both the payload and "resolution" or a tuple (payload, resolution)). Then update the branch that checks resolved.get("content_blob") to read the cached resolution from resolved_map (resolved.get("resolution") or unpack the tuple) and call with_content_blob using the appropriate content from ours_contents/theirs_contents based on that cached resolution, removing the per-key DB query and the call to frappe.get_all.frontend/src/pages/ContributionReview.vue (1)
146-158: Consider using radio inputs instead of checkboxes for mutually exclusive choices.Two checkboxes wired as a radio group can confuse users. A radio-style control (or a segmented button group) would better communicate that exactly one option must be selected. This is a minor UX nit given that
setResolutionhandles the toggle correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/ContributionReview.vue` around lines 146 - 158, The two mutually exclusive FormControl checkboxes should be replaced with radio inputs (or a segmented button group) to reflect that only one resolution can be selected; update the two FormControl instances (currently type="checkbox") to type="radio" (or use a dedicated RadioGroup/Segmented component) so their :modelValue binds to resolutions[conflict.name] and their `@update`:modelValue calls setResolution(conflict.name, 'ours'|'theirs') with the corresponding value, ensuring the radio inputs share the same name/key (conflict.name) so selection is exclusive and UI semantics match the existing setResolution 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 `@frontend/src/pages/ContributionReview.vue`:
- Around line 449-472: The loop in handleResolveAndMerge calling
resolveResource.submit for each item is not idempotent and can leave the user
stuck if it fails mid-way; update handleResolveAndMerge to either skip conflicts
that already have a client-side resolution (check resolutions[conflict.name] or
a resolved flag on conflict) before calling resolveResource.submit, or catch and
ignore the specific backend error string "Conflict is already resolved." inside
the per-conflict loop (log or continue) so the loop proceeds to the remaining
conflicts; ensure resolveResource.submit and conflicts/resolutions references
are used to locate the change and keep the outer try/catch for other errors and
finalizing resolvingMerge.value as before.
- Around line 440-446: Replace fragile substring matching of the user-facing
message with a structured error check: in the block that currently reads const
msg = error.messages?.[0] || ''; if (msg.includes('Merge conflicts detected')) {
await fetchConflicts(); } ..., change it to inspect a dedicated error field
(e.g., error.error_type or error.code) and call fetchConflicts() only when that
equals 'merge_conflicts' (falling back to showing toast.error(msg || __('Error
merging change request')) if no structured code is present). Update the
condition around fetchConflicts(), keep the existing toast.error fallback using
msg or the i18n string, and ensure you reference the same symbols (msg, error,
fetchConflicts, toast.error) when making the change.
In `@wiki/frappe_wiki/doctype/wiki_change_request/wiki_change_request.py`:
- Around line 856-862: The tuple returned from merge_items is being unpacked
into result and conflict_type but conflict_type is never used; change the
unpacking to use a deliberately unused variable name (e.g., _conflict_type) so
the intent is clear and static analysis warnings are suppressed: update the line
that currently reads "result, conflict_type = merge_items(...)" to use "result,
_conflict_type = merge_items(...)" while leaving the merge_items call and
subsequent handling of merged_items unchanged.
- Around line 749-771: In resolve_merge_conflict, validate the parent change
request's state before applying a resolution: after loading conflict =
frappe.get_doc("Wiki Merge Conflict", conflict_name) fetch the parent CR (e.g.,
cr = frappe.get_doc("Wiki Change Request", conflict.change_request)) and ensure
cr.status is one of allowed states (e.g., "Draft", "In Review"); if it is in a
finalized state like "Merged" or "Archived" raise frappe.throw(_("Cannot resolve
conflicts for a finalized Change Request."), frappe.ValidationError). Perform
this check just after loading the conflict and before computing
payload/resolution and saving the conflict.
- Around line 803-863: The merge loop currently silently drops keys when
merge_items(base, ours, theirs, ...) returns no result (new conflicts) for keys
not in resolved_map; instead, detect where conflict_type indicates a conflict
and for those keys create or update a "Wiki Merge Conflict" record (populate
change_request=name, doc_key=key, status="Conflict", resolution=None and attach
relevant base/ours/theirs metadata) and log a warning; do not silently swallow
the key—either leave it out of merged_items and ensure the conflict record
exists so the UI shows it, or raise an explicit retry/main-advanced error if you
prefer failing fast. Use the existing variables merged_items, resolved_map,
merge_items, conflict_type and the change request name to locate where to add
the conflict-creation logic.
---
Nitpick comments:
In `@frontend/src/pages/ContributionReview.vue`:
- Around line 146-158: The two mutually exclusive FormControl checkboxes should
be replaced with radio inputs (or a segmented button group) to reflect that only
one resolution can be selected; update the two FormControl instances (currently
type="checkbox") to type="radio" (or use a dedicated RadioGroup/Segmented
component) so their :modelValue binds to resolutions[conflict.name] and their
`@update`:modelValue calls setResolution(conflict.name, 'ours'|'theirs') with the
corresponding value, ensuring the radio inputs share the same name/key
(conflict.name) so selection is exclusive and UI semantics match the existing
setResolution logic.
In `@wiki/frappe_wiki/doctype/wiki_change_request/wiki_change_request.py`:
- Around line 827-849: The loop is re-querying "Wiki Merge Conflict" for each
resolved key to find the resolution side; instead, when building resolved_map
from resolved_conflicts (the data fetched into resolved_conflicts), include the
resolution field in each resolved_map entry (e.g., store a dict with both the
payload and "resolution" or a tuple (payload, resolution)). Then update the
branch that checks resolved.get("content_blob") to read the cached resolution
from resolved_map (resolved.get("resolution") or unpack the tuple) and call
with_content_blob using the appropriate content from
ours_contents/theirs_contents based on that cached resolution, removing the
per-key DB query and the call to frappe.get_all.
Comment on lines
440
to
446
| const msg = error.messages?.[0] || ''; | ||
| if (msg.includes('Merge conflicts detected')) { | ||
| await fetchConflicts(); | ||
| } else { | ||
| toast.error(msg || __('Error merging change request')); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "Merge conflicts detected" --type=pyRepository: frappe/wiki
Length of output: 196
🏁 Script executed:
# Also search for related conflict detection patterns in the backend
rg -n -i "conflict" --type=py | grep -i "error\|exception\|raise" | head -30Repository: frappe/wiki
Length of output: 690
🏁 Script executed:
# Check the three_way_merge function mentioned in the review scratchpad
rg -n "_three_way_merge" --type=py -A 10 -B 5Repository: frappe/wiki
Length of output: 5251
🏁 Script executed:
# Check how frappe.throw errors are structured in responses
# Look for error handling in the changeRequest createResource call
rg -n "messages" frontend/src/pages/ContributionReview.vue -B 3 -A 3Repository: frappe/wiki
Length of output: 1317
🏁 Script executed:
# Check if there's an error type or code field in Frappe error responses
rg -n "ValidationError\|PermissionError" wiki/frappe_wiki/doctype/wiki_change_request/wiki_change_request.pyRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
# Look at the merge_change_request method to understand the full flow
rg -n "def merge_change_request" --type=py -A 40Repository: frappe/wiki
Length of output: 4550
Use structured error indicator instead of fragile message text matching to detect merge conflicts.
Line 441 detects merge conflicts by checking if the error message includes 'Merge conflicts detected'. This is fragile because the message is internationalized with _() and wrapped in the messages array—if the backend message text changes, this detection silently breaks. Instead, have the backend return a structured error response with a specific error code or type field (e.g., error_type: 'merge_conflicts'), allowing the frontend to reliably detect conflict scenarios without depending on translatable user-facing text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/ContributionReview.vue` around lines 440 - 446, Replace
fragile substring matching of the user-facing message with a structured error
check: in the block that currently reads const msg = error.messages?.[0] || '';
if (msg.includes('Merge conflicts detected')) { await fetchConflicts(); } ...,
change it to inspect a dedicated error field (e.g., error.error_type or
error.code) and call fetchConflicts() only when that equals 'merge_conflicts'
(falling back to showing toast.error(msg || __('Error merging change request'))
if no structured code is present). Update the condition around fetchConflicts(),
keep the existing toast.error fallback using msg or the i18n string, and ensure
you reference the same symbols (msg, error, fetchConflicts, toast.error) when
making the change.
Place Keep Main / Keep Your Changes checkboxes in a grid-cols-2 layout above the DiffViewer so they align with their respective split-view columns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/pages/ContributionReview.vue (1)
459-466:expandedConflictsis not reset after a successful merge.After the success path sets
conflicts.value = []andhasConflicts.value = false, the stale conflict names remain inexpandedConflicts. While harmless today (the conflict UI is hidden), it leaks state if conflict detection is ever re-triggered in the same component lifecycle.🧹 Proposed cleanup
toast.success(__('Conflicts resolved and change request merged')); hasConflicts.value = false; conflicts.value = []; + expandedConflicts.clear(); + for (const key in resolutions) delete resolutions[key];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/ContributionReview.vue` around lines 459 - 466, The success path that clears conflicts (the block containing toast.success(...), hasConflicts.value = false, conflicts.value = [], changeRequest.reload(), and await changes.submit(...)) does not reset expandedConflicts; update that block to reset expandedConflicts to its initial empty state so stale names don't linger (e.g., if expandedConflicts is an array set expandedConflicts.value = [], or if it's a Set call expandedConflicts.value.clear()), placing the reset alongside the other resets after the merge succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/pages/ContributionReview.vue`:
- Around line 440-446: The current conflict detection inspects the user-facing
message text (msg.includes('Merge conflicts detected')), which is brittle;
instead, check a structured error field returned by the backend (e.g.
error.error_type, error.error_code or error.code) and branch on that value to
call fetchConflicts(); update the conditional in the catch block where msg and
fetchConflicts are used (referencing msg, error.messages, fetchConflicts, and
toast.error) to look for the structured error key and fall back to the existing
message only if the structured field is absent.
- Around line 449-457: handleResolveAndMerge currently submits each conflict
sequentially and will fail retrying if an earlier call succeeded (leaving the
user stuck); update the loop in handleResolveAndMerge to either skip conflicts
already marked resolved (e.g., check conflict.status or a resolved flag on
conflicts.value before calling resolveResource.submit) or wrap each
resolveResource.submit(...) call in a try/catch that suppresses the backend
"Conflict is already resolved." error and continues, and optionally replace the
loop with Promise.all over mapped submit promises to perform independent submits
in parallel while still handling per-item errors.
---
Nitpick comments:
In `@frontend/src/pages/ContributionReview.vue`:
- Around line 459-466: The success path that clears conflicts (the block
containing toast.success(...), hasConflicts.value = false, conflicts.value = [],
changeRequest.reload(), and await changes.submit(...)) does not reset
expandedConflicts; update that block to reset expandedConflicts to its initial
empty state so stale names don't linger (e.g., if expandedConflicts is an array
set expandedConflicts.value = [], or if it's a Set call
expandedConflicts.value.clear()), placing the reset alongside the other resets
after the merge succeeds.
- Replace fragile substring matching with exc_type check for conflict detection - Make handleResolveAndMerge idempotent by catching "already resolved" errors - Validate parent CR state before resolving conflicts (block finalized CRs) - Handle new conflicts arising during merge retry instead of silently dropping keys - Cache resolution side to avoid per-key DB queries in retry_merge_after_resolution - Rename unused conflict_type to _conflict_type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
NagariaHussain
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit