Skip to content

Commit 16df3e6

Browse files
authored
address review comments from #8019 (#9549)
<img width="1055" height="321" alt="image" src="https://github.com/user-attachments/assets/53ab00ed-4bd5-4fbf-990f-c4029b69541d" /> ## 📝 Summary Follow-up to #8019, which merged before a few unresolved review threads were addressed. This PR addresses the actionable items. Closes # ## 📋 Pre-Review Checklist - [ ] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [x] Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it. - [x] Video or media evidence is provided for any visual changes (optional). <!-- PR is more likely to be merged if evidence is provided for changes made --> ## ✅ Merge Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [x] Documentation has been updated where applicable, including docstrings for API changes. - [x] Tests have been added for the changes made. Made with [Cursor](https://cursor.com)
1 parent d1a9d89 commit 16df3e6

6 files changed

Lines changed: 49 additions & 88 deletions

File tree

frontend/src/components/editor/navigation/__tests__/clipboard.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ vi.mock("@/core/cells/pending-cut-service", () => ({
2828
}),
2929
usePendingCutState: () => ({
3030
cellIds: new Set(),
31-
clipboardData: null,
3231
}),
3332
}));
3433

frontend/src/components/editor/navigation/clipboard.ts

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,19 @@ const ClipboardCellDataSchema = z.object({
4242
version: z.literal("1.0"),
4343
});
4444

45-
function buildClipboardPayload(
46-
cells: Array<{ code: string; name?: string; config?: CellConfig }>,
47-
): { clipboardData: ClipboardCellData; plainText: string } {
45+
interface ClipboardCellInput {
46+
code: string;
47+
name?: string;
48+
config?: CellConfig;
49+
}
50+
51+
function toPlainText(cells: ClipboardCellInput[]): string {
52+
return cells.map((cell) => cell.code).join("\n\n");
53+
}
54+
55+
async function writeCellsToClipboard(
56+
cells: ClipboardCellInput[],
57+
): Promise<void> {
4858
const clipboardData: ClipboardCellData = {
4959
cells: cells.map((cell) => ({
5060
code: cell.code,
@@ -53,17 +63,9 @@ function buildClipboardPayload(
5363
})),
5464
version: "1.0",
5565
};
56-
const plainText = cells.map((cell) => cell.code).join("\n\n");
57-
return { clipboardData, plainText };
58-
}
59-
60-
async function writeCellsToClipboard(
61-
clipboardData: ClipboardCellData,
62-
plainText: string,
63-
): Promise<void> {
6466
const clipboardItem = new ClipboardItemBuilder()
6567
.add(MARIMO_CELL_MIMETYPE, clipboardData)
66-
.add("text/plain", plainText)
68+
.add("text/plain", toPlainText(cells))
6769
.build();
6870
await navigator.clipboard.write([clipboardItem]);
6971
}
@@ -80,22 +82,19 @@ export function useCellClipboard() {
8082
.filter(Boolean);
8183

8284
if (cells.length === 0) {
83-
// No cells to copy
8485
return;
8586
}
8687

87-
const { clipboardData, plainText } = buildClipboardPayload(cells);
88-
8988
try {
90-
await writeCellsToClipboard(clipboardData, plainText);
89+
await writeCellsToClipboard(cells);
9190
pendingCutActions.clear();
9291
toastSuccess(cells.length);
9392
} catch (error) {
9493
Logger.error("Failed to copy cells to clipboard", error);
9594

9695
// Fallback to simple text copy
9796
try {
98-
await copyToClipboard(plainText);
97+
await copyToClipboard(toPlainText(cells));
9998
pendingCutActions.clear();
10099
toastSuccess(cells.length);
101100
} catch {
@@ -110,21 +109,18 @@ export function useCellClipboard() {
110109
const cells = validCellIds.map((cellId) => notebook.cellData[cellId]);
111110

112111
if (cells.length === 0) {
113-
// No cells to cut
114112
return;
115113
}
116114

117-
const { clipboardData, plainText } = buildClipboardPayload(cells);
118-
119115
try {
120-
await writeCellsToClipboard(clipboardData, plainText);
121-
pendingCutActions.markForCut({ cellIds: validCellIds, clipboardData });
116+
await writeCellsToClipboard(cells);
117+
pendingCutActions.markForCut({ cellIds: validCellIds });
122118
} catch (error) {
123119
Logger.error("Failed to cut cells to clipboard", error);
124120
try {
125-
await copyToClipboard(plainText);
121+
await copyToClipboard(toPlainText(cells));
126122
// Mark cells as pending cut instead of deleting immediately
127-
pendingCutActions.markForCut({ cellIds: validCellIds, clipboardData });
123+
pendingCutActions.markForCut({ cellIds: validCellIds });
128124
} catch {
129125
toastError();
130126
}

frontend/src/core/cells/__tests__/cells.test.ts

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -622,7 +622,7 @@ describe("cell reducer", () => {
622622
before: false,
623623
});
624624
actions.createNewCell({
625-
cellId: "1" as CellId,
625+
cellId: cellId("1"),
626626
before: false,
627627
});
628628
expect(formatCells(state)).toMatchInlineSnapshot(`
@@ -637,8 +637,8 @@ describe("cell reducer", () => {
637637

638638
// Move first two cells after the third
639639
actions.moveCellsRelativeTo({
640-
cellIds: [firstCellId, "1" as CellId],
641-
targetCellId: "2" as CellId,
640+
cellIds: [firstCellId, cellId("1")],
641+
targetCellId: cellId("2"),
642642
position: "after",
643643
});
644644
expect(formatCells(state)).toMatchInlineSnapshot(`
@@ -658,7 +658,7 @@ describe("cell reducer", () => {
658658
before: false,
659659
});
660660
actions.createNewCell({
661-
cellId: "1" as CellId,
661+
cellId: cellId("1"),
662662
before: false,
663663
});
664664
expect(formatCells(state)).toMatchInlineSnapshot(`
@@ -682,14 +682,14 @@ describe("cell reducer", () => {
682682
{
683683
columnId: col.id,
684684
index: col.indexOfOrThrow(
685-
"1" as CellId,
685+
cellId("1"),
686686
) as import("@/utils/id-tree").CellIndex,
687687
},
688688
];
689689

690690
actions.moveCellsRelativeTo({
691-
cellIds: [firstCellId, "1" as CellId],
692-
targetCellId: "2" as CellId,
691+
cellIds: [firstCellId, cellId("1")],
692+
targetCellId: cellId("2"),
693693
position: "after",
694694
previousPlacements,
695695
});
@@ -721,7 +721,7 @@ describe("cell reducer", () => {
721721
before: false,
722722
});
723723
actions.createNewCell({
724-
cellId: "1" as CellId,
724+
cellId: cellId("1"),
725725
before: false,
726726
});
727727

@@ -736,14 +736,14 @@ describe("cell reducer", () => {
736736
{
737737
columnId: col.id,
738738
index: col.indexOfOrThrow(
739-
"1" as CellId,
739+
cellId("1"),
740740
) as import("@/utils/id-tree").CellIndex,
741741
},
742742
];
743743

744744
actions.moveCellsRelativeTo({
745-
cellIds: [firstCellId, "1" as CellId],
746-
targetCellId: "2" as CellId,
745+
cellIds: [firstCellId, cellId("1")],
746+
targetCellId: cellId("2"),
747747
position: "after",
748748
previousPlacements,
749749
});
@@ -757,7 +757,7 @@ describe("cell reducer", () => {
757757
"
758758
`);
759759

760-
actions.deleteCell({ cellId: "2" as CellId });
760+
actions.deleteCell({ cellId: cellId("2") });
761761
expect(formatCells(state)).toMatchInlineSnapshot(`
762762
"
763763
[0] ''

frontend/src/core/cells/__tests__/pending-cut-service.test.tsx

Lines changed: 14 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import { act, renderHook } from "@testing-library/react";
44
import { createStore, Provider } from "jotai";
55
import { describe, expect, it } from "vitest";
6+
import { cellId } from "@/__tests__/branded";
67
import type { CellId } from "@/core/cells/ids";
78
import {
89
pendingCutStateAtom,
@@ -20,15 +21,10 @@ function createTestWrapper() {
2021
return { wrapper, store };
2122
}
2223

23-
const mockClipboardData = {
24-
cells: [{ code: "x = 1", name: "cell1" }],
25-
version: "1.0" as const,
26-
};
27-
2824
describe("pending-cut-service", () => {
29-
it("markForCut sets cellIds and clipboardData", () => {
25+
it("markForCut sets cellIds", () => {
3026
const { wrapper, store } = createTestWrapper();
31-
const cellIds: CellId[] = ["cell-1" as CellId, "cell-2" as CellId];
27+
const cellIds: CellId[] = [cellId("cell-1"), cellId("cell-2")];
3228

3329
const { result } = renderHook(
3430
() => ({
@@ -39,20 +35,16 @@ describe("pending-cut-service", () => {
3935
);
4036

4137
act(() => {
42-
result.current.actions.markForCut({
43-
cellIds,
44-
clipboardData: mockClipboardData,
45-
});
38+
result.current.actions.markForCut({ cellIds });
4639
});
4740

4841
const state = store.get(pendingCutStateAtom);
4942
expect(state.cellIds).toEqual(new Set(cellIds));
50-
expect(state.clipboardData).toEqual(mockClipboardData);
5143
});
5244

5345
it("clear resets to initial state", () => {
5446
const { wrapper, store } = createTestWrapper();
55-
const cellIds: CellId[] = ["cell-1" as CellId];
47+
const cellIds: CellId[] = [cellId("cell-1")];
5648

5749
const { result } = renderHook(
5850
() => ({
@@ -63,10 +55,7 @@ describe("pending-cut-service", () => {
6355
);
6456

6557
act(() => {
66-
result.current.actions.markForCut({
67-
cellIds,
68-
clipboardData: mockClipboardData,
69-
});
58+
result.current.actions.markForCut({ cellIds });
7059
});
7160
expect(store.get(pendingCutStateAtom).cellIds.size).toBe(1);
7261

@@ -75,48 +64,40 @@ describe("pending-cut-service", () => {
7564
});
7665
const state = store.get(pendingCutStateAtom);
7766
expect(state.cellIds.size).toBe(0);
78-
expect(state.clipboardData).toBeNull();
7967
});
8068

8169
it("useIsPendingCut returns true when cellId is marked for cut", () => {
8270
const { wrapper } = createTestWrapper();
83-
const cellId = "cell-1" as CellId;
71+
const targetCellId = cellId("cell-1");
8472

8573
const { result: actionsResult } = renderHook(() => usePendingCutActions(), {
8674
wrapper,
8775
});
8876
const { result: isPendingResult } = renderHook(
89-
() => useIsPendingCut(cellId),
77+
() => useIsPendingCut(targetCellId),
9078
{ wrapper },
9179
);
9280

9381
expect(isPendingResult.current).toBe(false);
9482

9583
act(() => {
96-
actionsResult.current.markForCut({
97-
cellIds: [cellId],
98-
clipboardData: mockClipboardData,
99-
});
84+
actionsResult.current.markForCut({ cellIds: [targetCellId] });
10085
});
10186

10287
expect(isPendingResult.current).toBe(true);
10388
});
10489

10590
it("useIsPendingCut returns false when cellId is not marked for cut", () => {
10691
const { wrapper } = createTestWrapper();
107-
const { result } = renderHook(
108-
() => useIsPendingCut("other-cell" as CellId),
109-
{ wrapper },
110-
);
92+
const { result } = renderHook(() => useIsPendingCut(cellId("other-cell")), {
93+
wrapper,
94+
});
11195

11296
const { result: actionsResult } = renderHook(() => usePendingCutActions(), {
11397
wrapper,
11498
});
11599
act(() => {
116-
actionsResult.current.markForCut({
117-
cellIds: ["cell-1" as CellId],
118-
clipboardData: mockClipboardData,
119-
});
100+
actionsResult.current.markForCut({ cellIds: [cellId("cell-1")] });
120101
});
121102

122103
expect(result.current).toBe(false);
@@ -134,10 +115,7 @@ describe("pending-cut-service", () => {
134115
expect(hasPendingResult.current).toBe(false);
135116

136117
act(() => {
137-
actionsResult.current.markForCut({
138-
cellIds: ["cell-1" as CellId],
139-
clipboardData: mockClipboardData,
140-
});
118+
actionsResult.current.markForCut({ cellIds: [cellId("cell-1")] });
141119
});
142120

143121
expect(hasPendingResult.current).toBe(true);

frontend/src/core/cells/pending-cut-service.ts

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,33 @@
11
/* Copyright 2026 Marimo. All rights reserved. */
22

33
import { atom, useAtomValue } from "jotai";
4-
import type { ClipboardCellData } from "@/components/editor/navigation/clipboard";
54
import type { CellId } from "@/core/cells/ids";
65
import { createReducerAndAtoms } from "@/utils/createReducer";
76

87
interface PendingCutState {
98
cellIds: Set<CellId>;
10-
clipboardData: ClipboardCellData | null;
119
}
1210

1311
const initialState = (): PendingCutState => ({
1412
cellIds: new Set(),
15-
clipboardData: null,
1613
});
1714

1815
const {
1916
valueAtom: pendingCutStateAtom,
2017
useActions: usePendingCutActionsInternal,
2118
} = createReducerAndAtoms(initialState, {
22-
markForCut: (
23-
_state,
24-
action: { cellIds: CellId[]; clipboardData: ClipboardCellData },
25-
) => {
19+
markForCut: (_state, action: { cellIds: CellId[] }) => {
2620
return {
2721
cellIds: new Set(action.cellIds),
28-
clipboardData: action.clipboardData,
2922
};
3023
},
3124
clear: () => {
3225
return initialState();
3326
},
3427
});
3528

36-
// Re-export the state atom
3729
export { pendingCutStateAtom };
3830

39-
// Derived atom just for cell IDs (for easier consumption)
4031
export const pendingCutCellIdsAtom = atom(
4132
(get) => get(pendingCutStateAtom).cellIds,
4233
);

frontend/src/css/app/Cell.css

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -75,14 +75,11 @@
7575

7676
/* Styling for cells marked for cut */
7777
&.pending-cut {
78-
opacity: 0.6;
79-
border-style: dashed;
80-
border-color: var(--amber-7);
81-
8278
.output-area,
8379
.cm-gutters,
8480
.cm {
85-
background-color: var(--amber-2);
81+
background-color: var(--gray-2);
82+
opacity: 0.55;
8683
}
8784
}
8885

0 commit comments

Comments
 (0)