Add more merge options

[Jordonbc]

No description provided.

[github-actions]

Review findings

1. [BLOCKING] promptMergeStrategy permanently hangs when dismissed without selecting an option

Frontend/src/scripts/features/mergeStrategy.ts:47-52 creates a Promise<string | null> stored in pendingResolve. The promise is resolved only when a .merge-strategy-option element is clicked (lines 23–26).

Three dismiss paths bypass resolvePending entirely and never resolve the promise:

• Backdrop click — Frontend/src/scripts/ui/modals.ts:172-173 catches the click and calls closeWithAnimation → closeModal, which does not call resolvePending(null).
• Cancel button — same [data-close] path through the generic handler.
• Escape key — modals.ts:217-223 catches Escape and calls closeWithAnimation, same outcome.

In Frontend/src/scripts/features/branches.ts:217 the calling code does await promptMergeStrategy(...). If the modal is dismissed, this await hangs forever. The entire merge workflow is blocked with no way for the user to recover other than restarting the app.

Fix: Listen for the modal:closed event dispatched by setModalHidden (modals.ts:90) on the merge-strategy-modal element and call resolvePending(null):

// In mergeStrategy.ts, add after wireMergeStrategyModal or inside promptMergeStrategy:
const modal = document.getElementById('merge-strategy-modal');
if (modal) {
  modal.addEventListener('modal:closed', () => resolvePending(null));
}

This is guaranteed to fire on all three dismiss paths because setModalHidden dispatches the event whenever the modal transitions from visible to hidden.

Confidence: High

---

2. [IMPORTANT] Rebase hint describes the inverse direction of the operation

Frontend/src/modals/merge-strategy.html:18:

<p class="hint">Rebase the current branch onto the selected branch.</p>

The operation context is "Merge feature into main" (the user right-clicked branch feature, selected "Merge into current…"). The hint says to rebase the current branch onto the selected branch, which is git rebase feature while on main — rewriting main on top of feature. This is the opposite of what a user selecting "Merge into current" from the feature branch context menu would expect.

The correct description for a merge-rebase operation should be something like "Rebase the selected branch onto the current branch and fast-forward" (equivalent to git checkout feature && git rebase main && git checkout main && git merge feature).

Fix: Update the hint to accurately describe the direction, or remove the rebase option entirely if it is not actually implemented by any backend.

Confidence: High

---

3. [MINOR] Capability model cannot express partial strategy support

Backend/src/core/models.rs:240-244:

pub struct VcsCaps {
    pub merge_strategies: bool,
}

A single boolean means a backend either supports all of merge/squash/rebase or none. There is no way to express support for only squash (or only rebase). If any future backend implements squash but not rebase, the current VcsCaps forces it to either reject both (false negative for squash) or accept both (runtime failure for rebase).

Fix: Replace the boolean with a set or array of supported strategy names, or add per-strategy bools:

pub struct VcsCaps {
    pub strategies: Vec<String>,
}

This would need matching changes in the validation logic at branches.rs:423-424 to check membership instead of the single flag.

Confidence: Medium (design limitation, not a current breakage)

---

No other issues found

The validation, capability check, default reject in the trait, proxy forwarding, test harness updates, and frontend wiring logic are all internally consistent. The PR integrates cleanly with the existing plugin-based VCS architecture.

opencode session  |  github run

[github-actions]

Review findings

1. [BLOCKING] promptMergeStrategy permanently hangs when modal is dismissed

Frontend/src/scripts/features/mergeStrategy.ts:47-52 creates a Promise<string | null> whose resolver is stored in pendingResolve. The promise resolves only when a .merge-strategy-option is clicked (lines 23–26).

Three dismiss paths never touch resolvePending:

• Backdrop click — Frontend/src/scripts/ui/modals.ts:170-174 catches the click and calls closeWithAnimation → closeModal → setModalHidden(el, true), which dispatches modal:closed.
• Cancel button — same [data-close] path via modals.ts:173, same outcome.
• Escape key — modals.ts:217-223 catches Escape and calls closeWithAnimation, same outcome.

In Frontend/src/scripts/features/branches.ts:217 the caller does await promptMergeStrategy(...). If the modal is dismissed, this await never resolves. The entire "Merge into current…" context-menu action hangs with no recovery short of restarting the app.

Fix: Listen for the modal:closed event (dispatched by setModalHidden at modals.ts:90) on the merge-strategy-modal element and call resolvePending(null) in that listener:

const modal = document.getElementById('merge-strategy-modal');
modal?.addEventListener('modal:closed', () => resolvePending(null));

Confidence: High

---

2. [IMPORTANT] Rebase hint describes the opposite direction of the merge

Frontend/src/modals/merge-strategy.html:22:

<p class="hint">Rebase the current branch onto the selected branch.</p>

The operation context is "Merge feature into main" (user right-clicked feature, selected "Merge into current…"). The hint describes git rebase feature while on main — rewriting main on top of feature. This is the opposite of what "Merge into current" means for a user expecting the source branch's changes to be incorporated into the current branch.

A correct description for a rebase-as-merge-step would be "Rebase the selected branch onto the current branch, then fast-forward" (equivalent to git checkout feature && git rebase main && git checkout main && git merge feature).

Fix: Update the hint to match the actual merge direction, for example: "Rebase the selected branch's commits onto the current branch."

Confidence: High

---

3. [MINOR] VcsCaps cannot express partial strategy support

Backend/src/core/models.rs:240-244:

pub struct VcsCaps {
    pub merge_strategies: bool,
}

A single boolean forces a backend to either advertise support for all strategies (merge/squash/rebase) or none. There is no way to express support for, say, squash but not rebase. If any future backend implements only subset, the current VcsCaps forces either a false negative (rejecting squash when it would work) or a runtime failure (accepting rebase when the backend cannot handle it).

Fix: Replace the boolean with per-strategy fields or a strategy set:

pub struct VcsCaps {
    pub merge_squash: bool,
    pub merge_rebase: bool,
}

This would also require aligning the validation gate in branches.rs:438-442 to check the individual flag instead of the combined bool.

Confidence: Medium — design limitation, not a current breakage.

---

No other issues found

The validation logic (branches.rs:417-428), capability gating (branches.rs:430-443), default reject in the Vcs trait (core/mod.rs: merge_into_current_with_message), proxy forwarding (vcs_proxy.rs:306-315), test harness updates, and frontend wiring are all internally consistent and integrate cleanly with the existing plugin-based architecture.

opencode session  |  github run

[github-actions]

The prior reviews (both of which appear to have been generated from an intermediate commit before ee83bff "Fixed issues") found three issues. Let me verify each against the current HEAD:

• Finding 1 (Hanging promise): Already fixed in ee83bff. The current mergeStrategy.ts:54-58 registers a modal:closed listener that calls resolvePending(null) on any dismiss path.
• Finding 2 (Rebase hint direction): Already fixed in ee83bff. The current merge-strategy.html:22 reads "Rebase commits from the selected branch onto the current branch and fast-forward."
• Finding 3 (Capability model): Already fixed in ee83bff. merge_strategies is now Vec<String> with a Default impl, and validation gates check membership via iter().any().

Those issues are resolved. I found one remaining issue during review of the current state:

---

1. [MINOR] Doc comment on vcs_merge_strategies describes the wrong return type

Backend/src/tauri_commands/branches.rs:380-387:

/// Returns whether the current VCS backend supports merge strategy selection.
///
/// # Parameters
/// - `state`: Shared application state.
///
/// # Returns
/// - `Ok(bool)` indicating whether merge strategies are supported.
pub async fn vcs_merge_strategies(state: State<'_, AppState>) -> Result<Vec<String>, String> {

The doc comment says Ok(bool) and "whether" (implying a boolean), but the actual return type is Result<Vec<String>, String> — a list of supported strategy names. This is a copy-paste artifact from when the function was named vcs_merge_strategy_supported in an earlier commit.

Fix: Update the doc comment to match the returned type:

/// Returns the merge strategies advertised by the current VCS backend.
///
/// An empty list means only the default merge strategy is available.
///
/// # Returns
/// - `Ok(Vec<String>)` — supported strategy names (e.g. `"squash"`, `"rebase"`).

Confidence: High

---

No blocking or important issues remain. The integration is sound at both the Rust trait/plugin boundary and the Tauri IPC/frontend boundary. The validation, capability gating, default trait rejection, proxy forwarding, test harness updates, and frontend wiring are all internally consistent.

opencode session  |  github run

[github-actions]

1. [MINOR] Doc comment on vcs_merge_strategies describes wrong return type

Backend/src/tauri_commands/branches.rs:380-387:

/// Returns whether the current VCS backend supports merge strategy selection.
///
/// # Parameters
/// - `state`: Shared application state.
///
/// # Returns
/// - `Ok(bool)` indicating whether merge strategies are supported.
pub async fn vcs_merge_strategies(state: State<'_, AppState>) -> Result<Vec<String>, String> {

The doc comment says Ok(bool) and "whether" (implying a boolean), but the function returns Result<Vec<String>, String> — a list of strategy names. This was flagged in an earlier review (the third bot comment) but the fix was not applied.

Fix: Update the doc comment to match the return type:

/// Returns the merge strategies advertised by the current VCS backend.
///
/// An empty list means only the default merge strategy is available.
///
/// # Returns
/// - `Ok(Vec<String>)` — supported strategy names (e.g. `"squash"`, `"rebase"`).

Confidence: High

---

No blocking or important issues remain. The earlier issues (hanging promise in promptMergeStrategy, rebase hint direction, VcsCaps boolean model) were all fixed in commit ee83bff. The validation, capability gating, default trait rejection, proxy forwarding, test harness updates, and frontend wiring are all internally consistent and integrate cleanly with the existing plugin-based architecture.

opencode session  |  github run

[github-actions]

Review findings

No blocking or important issues were found. The PR integrates cleanly with the existing VCS architecture.

Minor — Strategy selector modal does not filter to only supported strategies

Frontend/src/modals/merge-strategy.html:12-23 — The modal always shows all three options (Simple merge, Squash, Rebase) regardless of which strategies the backend actually supports. The frontend determines to show the modal based on strategies.some(s => s === 'squash' || s === 'rebase') (Frontend/src/scripts/features/branches.ts:216), but if a backend only supports squash (not rebase), the rebase option is still clickable and produces a backend error at Backend/src/tauri_commands/branches.rs:427-430. The modal should either hide or disable unsupported options by comparing each data-strategy value against the advertised strategies list.

Minor — Event listener accumulates on repeated calls to promptMergeStrategy

Frontend/src/scripts/features/mergeStrategy.ts:54-58 — Each call to promptMergeStrategy registers a new modal:closed listener on the modal element. If the function is called while the previous promise is still pending (very unlikely in practice since the modal blocks UI), old listener references are lost and can never be removed. Each leaked listener fires once on close and calls resolvePending(null), which is a no-op since pendingResolve is already null. The leak is bounded by user actions and causes no behavioral harm, but can accumulate if the user repeatedly invokes merge operations in a long session.

---

All pre-existing issues in the test file ("base"/"from" parameter mismatch at line 282, "name"/"branch" mismatch at line 323, the assertionless vcs_merge_context test at line 308) were not introduced by this PR and are outside its scope.

opencode session  |  github run

[github-actions]

After thorough review of the current state of the PR (all 7 commits including ac9501b "Fixed identified issues"), I found no blocking or important issues.

The previously flagged concerns (hanging promise on modal dismiss, rebase hint direction, boolean VcsCaps model, doc comment mismatch) were all resolved by commit ee83bff. The integration across all four layers — Rust trait/default/proxy, Tauri IPC, frontend modal wiring, and plugin JSON-RPC — is internally consistent. The capability gating (caps.merge_strategies), validation, and event dispatch (modal:closed → resolvePending(null)) paths are all correctly wired.

No blocking or important issues remain.

opencode session  |  github run