From b62d5764126495d6188e5ef5a9b050e7007de81c Mon Sep 17 00:00:00 2001 From: Sam van Vuuren Date: Wed, 3 Jun 2026 14:52:28 -0400 Subject: [PATCH] fix(editor): restore unmerge option and move merge confirm into the editor (#691) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merged cells stopped showing the unmerge button in Source Editing mode: CellList filtered merged cells out in exactly the mode where the provider intentionally keeps them, so the "Cancel merge" button could never render. Remove that filter so merged cells render (greyed out) with the unmerge button again. Also replace the native VS Code merge-confirmation popup — which appeared in the bottom-right corner of the screen and wrongly claimed the action "cannot be undone" — with a small in-editor confirmation modal using reversible wording. confirmCellMerge no longer shows the native dialog (its child-cell guard is preserved), and the now-unused `message` payload field is dropped. Adds regression tests for unmerge-button visibility and the confirmation modal gating the merge. Fixes #691 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../codexCellEditorMessagehandling.ts | 65 +++---- types/index.d.ts | 1 - .../CodexCellEditor/CellContentDisplay.tsx | 62 +++++- .../src/CodexCellEditor/CellList.tsx | 17 +- .../CellContentDisplay.mergeConfirm.test.tsx | 173 +++++++++++++++++ .../CellList.mergedCellUnmerge.test.tsx | 178 ++++++++++++++++++ 6 files changed, 439 insertions(+), 57 deletions(-) create mode 100644 webviews/codex-webviews/src/CodexCellEditor/__tests___/CellContentDisplay.mergeConfirm.test.tsx create mode 100644 webviews/codex-webviews/src/CodexCellEditor/__tests___/CellList.mergedCellUnmerge.test.tsx diff --git a/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts b/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts index 39a560b99..546bc65e2 100644 --- a/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts +++ b/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts @@ -3121,7 +3121,7 @@ const messageHandlers: Record Promise { const typedEvent = event as Extract; - const { currentCellId, previousCellId, currentContent, previousContent, message } = typedEvent.content; + const { currentCellId, previousCellId, currentContent, previousContent } = typedEvent.content; debug("confirmCellMerge message received for cells:", { currentCellId, previousCellId }); @@ -3149,45 +3149,36 @@ const messageHandlers: Record Promise { - provider.refreshWebview(webviewPanel, document); - } - }); + // Call the existing merge handler + await messageHandlers.mergeCellWithPrevious({ + event: mergeEvent, + document, + webviewPanel, + provider, + updateWebview: () => { + provider.refreshWebview(webviewPanel, document); + } + }); - // Only merge in target if we're working with a source file - if (isSourceFile) { - const workspaceFolder = vscode.workspace.getWorkspaceFolder(document.uri); - if (!workspaceFolder) { - throw new Error("No workspace folder found"); - } - await provider.mergeMatchingCellsInTargetFile(currentCellId, previousCellId, document.uri.toString(), workspaceFolder); + // Only merge in target if we're working with a source file + if (isSourceFile) { + const workspaceFolder = vscode.workspace.getWorkspaceFolder(document.uri); + if (!workspaceFolder) { + throw new Error("No workspace folder found"); } + await provider.mergeMatchingCellsInTargetFile(currentCellId, previousCellId, document.uri.toString(), workspaceFolder); } } catch (error) { console.error("Error in confirmCellMerge:", error); diff --git a/types/index.d.ts b/types/index.d.ts index 4a62f3622..0221bb449 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -603,7 +603,6 @@ export type EditorPostMessages = previousCellId: string; currentContent: string; previousContent: string; - message: string; }; } | { diff --git a/webviews/codex-webviews/src/CodexCellEditor/CellContentDisplay.tsx b/webviews/codex-webviews/src/CodexCellEditor/CellContentDisplay.tsx index 7529e89ac..a32ed2630 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/CellContentDisplay.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/CellContentDisplay.tsx @@ -137,6 +137,13 @@ const CellContentDisplay: React.FC = React.memo( const [showSparkleButton, setShowSparkleButton] = useState(false); const [showAuthModal, setShowAuthModal] = useState(false); const [showOfflineModal, setShowOfflineModal] = useState(false); + // Pending merge awaiting in-editor confirmation (replaces the native VS Code popup) + const [pendingMerge, setPendingMerge] = useState<{ + currentCellId: string; + previousCellId: string; + currentContent: string; + previousContent: string; + } | null>(null); const [isLockButtonGlowing, setIsLockButtonGlowing] = useState(false); const [isLockButtonFlashing, setIsLockButtonFlashing] = useState(false); const [isResolvingStructure, setIsResolvingStructure] = useState(false); @@ -390,18 +397,27 @@ const CellContentDisplay: React.FC = React.memo( return; } - // Send confirmation request to VS Code instead of using window.confirm + // Open an in-editor confirmation modal instead of the native VS Code popup + // (which appears in the bottom-right corner of the screen, far from the cell). + setPendingMerge({ + currentCellId: currentCell.cellMarkers[0], + previousCellId: targetCell.cellMarkers[0], + currentContent: currentCell.cellContent, + previousContent: targetCell.cellContent, + }); + }; + + const handleConfirmMerge = () => { + if (!pendingMerge) return; vscode.postMessage({ command: "confirmCellMerge", - content: { - currentCellId: currentCell.cellMarkers[0], - previousCellId: targetCell.cellMarkers[0], - currentContent: currentCell.cellContent, - previousContent: targetCell.cellContent, - message: - "Are you sure you want to merge this cell with the previous non-merged cell? This action cannot be undone.", - }, + content: pendingMerge, } as any); + setPendingMerge(null); + }; + + const handleCancelMergeConfirmation = () => { + setPendingMerge(null); }; // Line numbers are always generated and shown at the beginning of each line @@ -977,6 +993,34 @@ const CellContentDisplay: React.FC = React.memo( )} + { + if (!open) handleCancelMergeConfirmation(); + }} + > + + + Merge with previous cell + + This cell will be merged into the previous + non-merged cell. You can reverse this later by + unmerging the cell. + + + + + + + + )} diff --git a/webviews/codex-webviews/src/CodexCellEditor/CellList.tsx b/webviews/codex-webviews/src/CodexCellEditor/CellList.tsx index d7a46e011..ad03f51b4 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/CellList.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/CellList.tsx @@ -157,24 +157,21 @@ const CellList: React.FC = ({ // State to track unresolved comments count for each cell const [cellCommentsCount, setCellCommentsCount] = useState>(new Map()); - // Filter out merged cells if we're in correction editor mode for source text const filteredTranslationUnits = useMemo(() => { let filtered = translationUnits; - // Filter out merged cells if we're in correction editor mode for source text - if (isSourceText && isCorrectionEditorMode) { - filtered = filtered.filter((unit) => { - // Check if cell has merged metadata in the data property - const cellData = unit.data as any; - return !cellData?.merged; - }); - } + // NOTE: Do NOT filter out merged cells here. In source + correction editor mode the + // provider intentionally keeps merged cells in the translation units (see + // mergeRangesAndProcess) so they render greyed-out with the "unmerge" button. In every + // other mode the provider already strips merged cells, so they never reach this list. + // Filtering them here previously hid them in correction mode, which removed the only + // place the unmerge button could appear (issue #691). // Filter out milestone cells from the view (they remain in JSON) filtered = filtered.filter((unit) => unit.cellType !== CodexCellTypes.MILESTONE); return filtered; - }, [translationUnits, isSourceText, isCorrectionEditorMode]); + }, [translationUnits]); // Use filtered units for all operations const workingTranslationUnits = filteredTranslationUnits; // State to track completed translations (only successful ones) - REMOVED: Now handled by parent diff --git a/webviews/codex-webviews/src/CodexCellEditor/__tests___/CellContentDisplay.mergeConfirm.test.tsx b/webviews/codex-webviews/src/CodexCellEditor/__tests___/CellContentDisplay.mergeConfirm.test.tsx new file mode 100644 index 000000000..8af051c59 --- /dev/null +++ b/webviews/codex-webviews/src/CodexCellEditor/__tests___/CellContentDisplay.mergeConfirm.test.tsx @@ -0,0 +1,173 @@ +import React from "react"; +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { render, screen, fireEvent } from "@testing-library/react"; +import { QuillCellContent } from "../../../../../types"; +import { CodexCellTypes } from "../../../../../types/enums"; +import CellContentDisplay from "../CellContentDisplay"; + +// Regression test for the #691 follow-up: the merge confirmation is now an in-editor modal +// (rendered inside the source editor) instead of a native VS Code popup in the screen corner. +// Clicking the merge button must open the modal and NOT post `confirmCellMerge` until the user +// confirms in the modal. + +const mockVscode = { + postMessage: vi.fn(), + getState: vi.fn(), + setState: vi.fn(), +}; + +Object.defineProperty(window, "vscodeApi", { + value: mockVscode, + writable: true, +}); + +global.acquireVsCodeApi = vi.fn().mockReturnValue(mockVscode); + +vi.mock("@sharedUtils", () => ({ + shouldDisableValidation: vi.fn().mockReturnValue(false), +})); + +vi.mock("../contextProviders/UnsavedChangesContext", () => ({ + default: React.createContext({ + setUnsavedChanges: vi.fn(), + showFlashingBorder: false, + unsavedChanges: false, + toggleFlashingBorder: vi.fn(), + }), +})); + +vi.mock("../contextProviders/TooltipContext", () => ({ + useTooltip: () => ({ + showTooltip: vi.fn(), + hideTooltip: vi.fn(), + }), +})); + +vi.mock("../hooks/useCentralizedMessageDispatcher", () => ({ + useMessageHandler: vi.fn(() => {}), +})); + +vi.mock("../lib/audioController", () => ({ + globalAudioController: { + playExclusive: vi.fn().mockResolvedValue(undefined), + addListener: vi.fn(), + removeListener: vi.fn(), + }, +})); + +vi.mock("../lib/audioCache", () => ({ + getCachedAudioDataUrl: vi.fn().mockReturnValue(null), + setCachedAudioDataUrl: vi.fn(), +})); + +vi.mock("../ValidationButton", () => ({ + default: () =>
, +})); + +vi.mock("../AudioValidationButton", () => ({ + default: () => ( +
+ ), +})); + +vi.mock("../CommentsBadge", () => ({ + default: () =>
, +})); + +vi.mock("react-markdown", () => ({ + default: ({ children }: { children: React.ReactNode }) =>
{children}
, +})); + +const createMockCell = (cellId: string, content: string): QuillCellContent => ({ + cellMarkers: [cellId], + cellContent: content, + cellType: CodexCellTypes.TEXT, + editHistory: [ + { + editMap: ["value"], + value: content, + author: "test-user", + validatedBy: [], + timestamp: Date.now(), + type: "user-edit" as any, + }, + ], + cellLabel: cellId, + timestamps: { startTime: 0, endTime: 5 }, +}); + +const renderSecondCell = () => { + const firstCell = createMockCell("cell-1", "

First

"); + const secondCell = createMockCell("cell-2", "

Second

"); + const props = { + cell: secondCell, + vscode: mockVscode as any, + textDirection: "ltr" as const, + isSourceText: true, + hasDuplicateId: false, + highlightedCellId: null, + scrollSyncEnabled: true, + lineNumber: "2", + label: "Test Label", + lineNumbersEnabled: true, + isInTranslationProcess: false, + translationState: null as any, + allTranslationsComplete: false, + handleCellClick: vi.fn(), + audioAttachments: { "cell-2": "available" as const }, + currentUsername: "test-user", + requiredValidations: 1, + requiredAudioValidations: 1, + isCorrectionEditorMode: true, + translationUnits: [firstCell, secondCell], + }; + return render(); +}; + +const clickMergeButton = (container: HTMLElement) => { + const mergeButton = container.querySelector(".codicon-merge")?.closest("button"); + expect(mergeButton).toBeTruthy(); + fireEvent.click(mergeButton!); +}; + +const confirmCellMergeCalls = () => + mockVscode.postMessage.mock.calls.filter(([msg]) => msg?.command === "confirmCellMerge"); + +describe("CellContentDisplay - merge confirmation modal (#691 follow-up)", () => { + beforeEach(() => { + vi.clearAllMocks(); + Element.prototype.scrollIntoView = vi.fn(); + }); + + it("opens the in-editor modal on merge click and does not post confirmCellMerge yet", () => { + const { container } = renderSecondCell(); + clickMergeButton(container); + + // Modal is shown... + expect(screen.getByText("Merge with previous cell")).toBeTruthy(); + // ...but nothing is committed until the user confirms. + expect(confirmCellMergeCalls()).toHaveLength(0); + }); + + it("posts confirmCellMerge (without a message field) when the merge is confirmed", () => { + const { container } = renderSecondCell(); + clickMergeButton(container); + fireEvent.click(screen.getByRole("button", { name: "Merge" })); + + const calls = confirmCellMergeCalls(); + expect(calls).toHaveLength(1); + const msg = calls[0][0]; + expect(msg.content.currentCellId).toBe("cell-2"); + expect(msg.content.previousCellId).toBe("cell-1"); + expect(msg.content).not.toHaveProperty("message"); + }); + + it("does not post confirmCellMerge when the modal is cancelled", () => { + const { container } = renderSecondCell(); + clickMergeButton(container); + fireEvent.click(screen.getByRole("button", { name: "Cancel" })); + + expect(confirmCellMergeCalls()).toHaveLength(0); + expect(screen.queryByText("Merge with previous cell")).toBeNull(); + }); +}); diff --git a/webviews/codex-webviews/src/CodexCellEditor/__tests___/CellList.mergedCellUnmerge.test.tsx b/webviews/codex-webviews/src/CodexCellEditor/__tests___/CellList.mergedCellUnmerge.test.tsx new file mode 100644 index 000000000..6ac68c348 --- /dev/null +++ b/webviews/codex-webviews/src/CodexCellEditor/__tests___/CellList.mergedCellUnmerge.test.tsx @@ -0,0 +1,178 @@ +import React from "react"; +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { render } from "@testing-library/react"; +import { QuillCellContent } from "../../../../../types"; +import { CodexCellTypes } from "../../../../../types/enums"; +import CellList from "../CellList"; + +// Regression test for issue #691: "merged content no longer shows the option to unmerge". +// +// In source + correction-editor mode the provider intentionally keeps merged cells in the +// translation units so they render greyed-out with an "unmerge" (Cancel merge) button. +// A filter in CellList used to strip merged cells in exactly that mode, which removed the +// only place the unmerge button could appear. This test locks in that merged cells survive +// to render and expose the unmerge control. + +// Mock the VSCode API +const mockVscode = { + postMessage: vi.fn(), + getState: vi.fn(), + setState: vi.fn(), +}; + +Object.defineProperty(window, "vscodeApi", { + value: mockVscode, + writable: true, +}); + +global.acquireVsCodeApi = vi.fn().mockReturnValue(mockVscode); + +vi.mock("@sharedUtils", () => ({ + shouldDisableValidation: vi.fn().mockReturnValue(false), +})); + +vi.mock("../contextProviders/UnsavedChangesContext", () => ({ + default: React.createContext({ + setUnsavedChanges: vi.fn(), + showFlashingBorder: false, + unsavedChanges: false, + toggleFlashingBorder: vi.fn(), + }), +})); + +vi.mock("../contextProviders/TooltipContext", () => ({ + useTooltip: () => ({ + showTooltip: vi.fn(), + hideTooltip: vi.fn(), + }), +})); + +vi.mock("../hooks/useCentralizedMessageDispatcher", () => ({ + useMessageHandler: vi.fn(() => {}), +})); + +vi.mock("../lib/audioController", () => ({ + globalAudioController: { + playExclusive: vi.fn().mockResolvedValue(undefined), + addListener: vi.fn(), + removeListener: vi.fn(), + }, +})); + +vi.mock("../lib/audioCache", () => ({ + getCachedAudioDataUrl: vi.fn().mockReturnValue(null), + setCachedAudioDataUrl: vi.fn(), +})); + +vi.mock("../ValidationButton", () => ({ + default: () =>
, +})); + +vi.mock("../AudioValidationButton", () => ({ + default: () => ( +
+ ), +})); + +vi.mock("../CommentsBadge", () => ({ + default: () =>
, +})); + +vi.mock("react-markdown", () => ({ + default: ({ children }: { children: React.ReactNode }) =>
{children}
, +})); + +const createCell = ( + uuid: string, + content: string, + overrides: Partial = {} +): QuillCellContent => ({ + cellMarkers: [uuid], + cellContent: content, + cellType: CodexCellTypes.TEXT, + data: {}, + editHistory: [ + { + editMap: ["value"], + value: content, + author: "test-user", + validatedBy: [], + timestamp: Date.now(), + type: "user-edit" as any, + }, + ], + cellLabel: uuid, + timestamps: { startTime: 0, endTime: 5 }, + ...overrides, +}); + +const baseProps = (translationUnits: QuillCellContent[], isCorrectionEditorMode: boolean) => ({ + translationUnits, + fullDocumentTranslationUnits: translationUnits, + contentBeingUpdated: { + cellMarkers: [], + cellContent: "", + cellChanged: false, + }, + setContentBeingUpdated: vi.fn(), + handleCloseEditor: vi.fn(), + handleSaveHtml: vi.fn(), + vscode: mockVscode, + textDirection: "ltr" as const, + isSourceText: true, + isCorrectionEditorMode, + windowHeight: 800, + headerHeight: 100, + highlightedCellId: null, + scrollSyncEnabled: true, + currentUsername: "test-user", + requiredValidations: 1, + milestoneIndex: null, + currentMilestoneIndex: 0, + currentSubsectionIndex: 0, + cellsPerPage: 50, +}); + +describe("CellList - merged cell unmerge button (issue #691)", () => { + beforeEach(() => { + vi.clearAllMocks(); + Element.prototype.scrollIntoView = vi.fn(); + }); + + it("renders a merged cell with the unmerge button in source + correction editor mode", () => { + const translationUnits: QuillCellContent[] = [ + createCell("uuid-1", "

First cell

"), + createCell("uuid-2", "

Merged cell

", { + merged: true, + data: { merged: true }, + }), + ]; + + const { container } = render( + + ); + + // The merged cell content must still render (not filtered out)... + expect(container.textContent).toContain("Merged cell"); + // ...and it must expose the unmerge ("Cancel merge") control. + expect(container.querySelector('[title="Cancel merge"]')).not.toBeNull(); + }); + + it("does not show the unmerge button when not in correction editor mode", () => { + // Outside correction mode the provider never sends merged cells, but even if one slipped + // through, the unmerge button should only appear in correction mode. + const translationUnits: QuillCellContent[] = [ + createCell("uuid-1", "

First cell

"), + createCell("uuid-2", "

Merged cell

", { + merged: true, + data: { merged: true }, + }), + ]; + + const { container } = render( + + ); + + expect(container.querySelector('[title="Cancel merge"]')).toBeNull(); + }); +});