[FEAT] Add undo/redo support with history panel #50
Conversation
Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
|
@coderabbitai could you also review this? |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR adds comprehensive undo/redo functionality to the MergeNB conflict resolver. It introduces a configuration option to enable keyboard shortcuts, implements snapshot-based history state management in the UI component, adds row drag-and-drop support for reordering, and includes new test utilities and integration tests to validate the undo/redo workflows. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as ConflictResolver UI
participant History as History State
participant Snapshot as Snapshot Manager
User->>UI: Perform action (select, edit, move)
activate UI
UI->>Snapshot: Create snapshot of current state
activate Snapshot
Snapshot-->>UI: Snapshot created
deactivate Snapshot
UI->>History: Record history entry
activate History
History->>History: Add entry to history list
History-->>UI: History updated
deactivate History
UI->>User: Display updated UI + history panel
deactivate UI
User->>UI: Press Ctrl+Z (Undo)
activate UI
UI->>History: Get previous history entry
activate History
History-->>UI: Return prior snapshot
deactivate History
UI->>Snapshot: Apply snapshot to state
activate Snapshot
Snapshot->>UI: Restore previous state
deactivate Snapshot
UI->>User: Display restored UI
deactivate UI
User->>UI: Press Ctrl+Shift+Z (Redo)
activate UI
UI->>History: Get next history entry
activate History
History-->>UI: Return next snapshot
deactivate History
UI->>Snapshot: Apply snapshot to state
activate Snapshot
Snapshot->>UI: Restore forward state
deactivate Snapshot
UI->>User: Display restored UI
deactivate UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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. ✨ 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 |
…olution UI - Fixes history panel - Adds higher quality tests - Merge rows are now draggable (untested)
Avni2000
force-pushed
the
copilot/add-undo-redo-actions-again
branch
from
2abeb7f to
f46f71b
Compare
|
Closes #40 |
|
@coderabbitai sorry for force pushing! Try again. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/tests/undoRedoActions.test.ts`:
- Around line 98-100: Move the minimum-conflict-count guard into the start of
the run() test so we fail fast with a clear message: relocate the check that
throws when conflictCount < 2 (currently used later around Action 6) up to the
top of run() (near where conflictCount is first evaluated around line 98) so
actions 2–3 won't run when there is only one conflict; update the thrown message
to indicate "Expected at least two conflict rows" and remove or skip the later
duplicate guard at Action 6.
🧹 Nitpick comments (6)
src/web/client/styles.ts (1)
329-340: Duplicate.cell-placeholderrule blocks with conflicting values.The
.cell-placeholderselector is defined twice (lines 329–340 and 913–924) with differentcolor,border, andbackgroundvalues. The second block silently overrides the first. Consider removing the first block or consolidating them to avoid confusion.Also applies to: 913-924
src/web/webTypes.ts (1)
32-33:enableUndoRedoHotkeysmissing fromWebConflictData.The field is added to
UnifiedConflictbut not toWebConflictData(line 82), which is described as the "web-compatible version of UnifiedConflict." The payload sent inWebConflictPanel._sendConflictData()includes this field, soWebConflictDatashould carry it too for type-level consistency.Proposed fix
// Display options hideNonConflictOutputs?: boolean; showCellHeaders?: boolean; + enableUndoRedoHotkeys?: boolean; // Branch informationsrc/tests/integrationUtils.ts (1)
134-150: Minor: redundant initial poll before loop body.
waitForResolvedCountcallsgetResolvedCountat line 141 then immediately again at line 143 on the first loop iteration, discarding the first result. The same pattern exists inwaitForAllConflictsResolvedabove, so this is consistent — but both could short-circuit with an early return before the loop when the initial poll already matches.♻️ Optional improvement
): Promise<{ resolved: number; total: number }> { const start = Date.now(); let last = await getResolvedCount(page); + if (last.resolved === expectedResolved) { + return last; + } while (Date.now() - start < timeoutMs) { - last = await getResolvedCount(page); + await new Promise(r => setTimeout(r, pollMs)); + last = await getResolvedCount(page); if (last.resolved === expectedResolved) { return last; } - await new Promise(r => setTimeout(r, pollMs)); } return last; }src/tests/undoRedoActions.test.ts (1)
17-17: Prefer importingMergeSidefromintegrationUtilsinstead of redeclaring.This type is already exported from
./integrationUtils(line 5 of that file). Redeclaring it here introduces a maintenance risk if the type definition diverges.♻️ Suggested fix
import { clickHistoryUndo, clickHistoryRedo, waitForResolvedCount, + type MergeSide, } from './integrationUtils'; - -type MergeSide = 'base' | 'current' | 'incoming';src/web/client/ConflictResolver.tsx (2)
476-501: Side-effectingapplySnapshotinsidesetHistoryupdater is a code smell.
handleUndo,handleRedo, andhandleJumpToHistoryall callapplySnapshot(which dispatches 8 separatesetStatecalls) inside thesetHistoryupdater function. React state updaters should ideally be pure — they receive previous state and return next state. While React batches the updates and this works correctly in practice (including Strict Mode sinceapplySnapshotis idempotent), it violates the convention and makes the state flow harder to reason about.Consider restructuring so
applySnapshotis called aftersetHistory, e.g.:♻️ Suggested refactor sketch
const handleUndo = useCallback(() => { - setHistory(prev => { - if (prev.index === 0) return prev; - const nextIndex = prev.index - 1; - applySnapshot(prev.entries[nextIndex].snapshot); - return { ...prev, index: nextIndex }; - }); + setHistory(prev => { + if (prev.index === 0) return prev; + return { ...prev, index: prev.index - 1 }; + }); }, [applySnapshot]); + + // Sync snapshot whenever history.index changes + useEffect(() => { + applySnapshot(history.entries[history.index].snapshot); + }, [history.index]);This would need care to avoid infinite loops (since
applySnapshotsets state that triggers re-renders), so the effect would need to compare the index to a ref to avoid re-applying. An alternative is to useuseReducerto manage all state atomically.
409-418:handleCommitContentreadshistoryfrom closure — dependency array is correct but recreates on every history change.Since
historyis in the dependency array,handleCommitContent(and therefore theonBlurcallback) is recreated on every history state change (every undo/redo/record). This causesMergeRowto re-render on every history change even for rows whose resolution state hasn't changed.Consider reading history via a ref instead (same pattern used for
choices,rows, etc.) to keep this callback stable:♻️ Suggested improvement
+ const historyRef = useRef(history); + useEffect(() => { historyRef.current = history; }, [history]); + const handleCommitContent = useCallback((index: number) => { const current = choicesRef.current.get(index); if (!current) return; - const lastSnapshot = history.entries[history.index]?.snapshot; + const lastSnapshot = historyRef.current.entries[historyRef.current.index]?.snapshot; const lastChoice = lastSnapshot?.choices.get(index); if (lastChoice && lastChoice.resolvedContent === current.resolvedContent && lastChoice.choice === current.choice) { return; } recordHistory(`Edit conflict ${index + 1}`, choicesRef.current, rowsRef.current); - }, [history, recordHistory]); + }, [recordHistory]);
| if (conflictCount === 0) { | ||
| throw new Error('Expected at least one conflict row'); | ||
| } |
There was a problem hiding this comment.
The minimum-conflict-count guard should be at the top of run().
Action 6 (line 227) throws if conflictCount < 2, but earlier actions (2–3) already implicitly require multiple conflicts — if there's only one, Action 2's redo leaves firstRow in a "deleted" state, and Action 3 (line 137) will time out waiting for .resolved-content-input on it. Move the conflictCount < 2 guard up to line 98 to fail early with a clear message.
♻️ Suggested fix
- if (conflictCount === 0) {
- throw new Error('Expected at least one conflict row');
+ if (conflictCount < 2) {
+ throw new Error('Expected at least two conflict rows for undo/redo test');
}Also applies to: 227-229
🤖 Prompt for AI Agents
In `@src/tests/undoRedoActions.test.ts` around lines 98 - 100, Move the
minimum-conflict-count guard into the start of the run() test so we fail fast
with a clear message: relocate the check that throws when conflictCount < 2
(currently used later around Action 6) up to the top of run() (near where
conflictCount is first evaluated around line 98) so actions 2–3 won't run when
there is only one conflict; update the thrown message to indicate "Expected at
least two conflict rows" and remove or skip the later duplicate guard at Action
6.
|
@coderabbitai can you also investigate why the websocket closes in CI? Again, haven't had a chance to fully test this PR/actually review it, but if you could do some preliminary research, that'd be great. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 19805 🏁 Script executed: Length of output: 14683 🏁 Script executed: Length of output: 12380 🏁 Script executed: Length of output: 34400 🏁 Script executed: Length of output: 18872 🏁 Script executed: Length of output: 2038 🏁 Script executed: Length of output: 3997
Root Cause AnalysisThe test Timeline of the failure:
Why This HappensThis appears to be a browser crash or instability issue during complex drag-and-drop operations in headless Chromium, particularly in CI environments. Contributing factors:
The "firewall warning" you mentioned in the PR description is unrelated—that's about blocking Recommended Fixes1. Add explicit stabilization delays after drag operations// In undoRedoActions.test.ts around line 208
await sourceCell.dragTo(targetPlaceholder);
await page.waitForTimeout(500); // Give browser time to stabilize2. Increase timeouts for drag-related operationsawait movedCell.waitFor({ timeout: 10000 }); // Increase from 50003. Add retry logic for flaky drag operationsasync function dragWithRetry(source: Locator, target: Locator, retries = 2) {
for (let i = 0; i < retries; i++) {
try {
await source.dragTo(target);
return;
} catch (e) {
if (i === retries - 1) throw e;
await page.waitForTimeout(1000);
}
}
}4. Use Playwright's more reliable drag APIInstead of const sourceBbox = await sourceCell.boundingBox();
const targetBbox = await targetPlaceholder.boundingBox();
if (sourceBbox && targetBbox) {
await page.mouse.move(sourceBbox.x + sourceBbox.width / 2, sourceBbox.y + sourceBbox.height / 2);
await page.mouse.down();
await page.mouse.move(targetBbox.x + targetBbox.width / 2, targetBbox.y + targetBbox.height / 2, { steps: 10 });
await page.mouse.up();
}5. Capture browser console logs for debuggingAdd at the start of the test: page.on('console', msg => console.log('Browser console:', msg.text()));
page.on('pageerror', err => console.error('Browser error:', err));6. Consider making drag tests more resilient or optionalAdd a try-catch around the drag test section: try {
console.log('\n=== Undo/Redo: Move Unmatched Cell ===');
// ... drag test code ...
} catch (error) {
console.warn('Cell move test failed (known flaky in CI):', error.message);
// Continue with other tests
}Quick Fix to Unblock CIThe simplest short-term fix would be to skip the problematic drag test in CI: if (!process.env.CI) {
console.log('\n=== Undo/Redo: Move Unmatched Cell ===');
// ... drag test code ...
} else {
console.log('\n=== Skipping cell move test in CI (known flaky) ===');
}Would you like me to help implement any of these fixes? The combination of stabilization delays ( |
Avni2000
changed the title
|
@coderabbitai make an issue so we can decide later whether or not to actually keep the cell drag option for unified cells as well. If so, we need to add a proper drop zone for it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 72
The issue outlines the decision points and references this PR and discussion. You can refine the details as needed when you're ready to address it. 🧠 Learnings used |
New Features
mergeNB.ui.enableUndoRedoHotkeysto control keyboard shortcut availability (default: enabled).Tests