Skip to content

Commit 304f7b9

Browse files
authored
fix component text corruption in duplicate dialog (#1448)
## Description Closes #1403 Refactored component duplication and library management to use hydrated component references. This PR eliminates the need to repeatedly serialize/deserialize components with `js-yaml`. Key changes: - Added a `replaceComponentName` helper function to modify component names directly in the text - Updated `ComponentDuplicateDialog` to work with hydrated components - Modified `FavoriteComponentToggle` to hydrate components before adding them to the library - Updated the component library provider to handle hydrated components throughout ## Type of Change - [x] Improvement - [x] Cleanup/Refactor ## Checklist - [x] I have tested this does not break current pipelines / runs functionality - [ ] I have tested the changes on staging ## Test Instructions [Screen Recording 2025-12-01 at 8.29.44 PM.mov <span class="graphite__hidden">(uploaded via Graphite)</span> <img class="graphite__hidden" src="https://app.graphite.com/user-attachments/thumbnails/06a293d2-a1a9-47b8-9fec-920c3f82b312.mov" />](https://app.graphite.com/user-attachments/video/06a293d2-a1a9-47b8-9fec-920c3f82b312.mov) 1. Follow video to test the functionality. 2. Open any component for edit in YAML editor (not Python). 3. Add multiple lines of strings (e.g. `#1`​ like comments) 4. Save. Observe the Duplicate dialog. Test that dialog works exactly same as in staging/production 5. Ensure, that after subsequent edits of the same component does not alterate the content of component (adding new lines, or collapsing lines into one line). 6. Follow examples from the ticket #1403 7. Ensure that remaining functionality has no regressions (e.g. adding components via Canvas drops, adding removing components from favorites). ## Notes - This PR does not address round-trip issues with "Component Details" e.g. download YAML or Implementation Code (which comes from Spec, not Text).
1 parent a687fcf commit 304f7b9

File tree

7 files changed

+485
-117
lines changed

7 files changed

+485
-117
lines changed
Lines changed: 280 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,280 @@
1+
import {
2+
act,
3+
fireEvent,
4+
render,
5+
screen,
6+
waitFor,
7+
} from "@testing-library/react";
8+
import { beforeEach, describe, expect, test, vi } from "vitest";
9+
10+
import type { HydratedComponentReference } from "@/utils/componentSpec";
11+
import type { UserComponent } from "@/utils/localforage";
12+
13+
import ComponentDuplicateDialog from "./ComponentDuplicateDialog";
14+
15+
// Mock only what's necessary - file operations and digest generation
16+
vi.mock("@/utils/componentStore", async (importOriginal) => ({
17+
...(await importOriginal()),
18+
deleteComponentFileFromList: vi.fn().mockResolvedValue(undefined),
19+
}));
20+
21+
// Import mocked functions
22+
import { deleteComponentFileFromList } from "@/utils/componentStore";
23+
24+
describe("ComponentDuplicateDialog", () => {
25+
// Test data
26+
const mockExistingComponent: UserComponent = {
27+
name: "ExistingComponent",
28+
componentRef: {
29+
name: "ExistingComponent",
30+
digest: "existing-digest-123",
31+
},
32+
data: new ArrayBuffer(0),
33+
creationTime: new Date("2024-01-01"),
34+
modificationTime: new Date("2024-01-02"),
35+
};
36+
37+
const mockNewComponent: HydratedComponentReference = {
38+
name: "NewComponent",
39+
digest: "new-digest-456",
40+
text: `name: NewComponent
41+
description: A new component
42+
inputs: []
43+
outputs: []
44+
implementation:
45+
container:
46+
image: test-image`,
47+
spec: {
48+
name: "NewComponent",
49+
description: "A new component",
50+
inputs: [],
51+
outputs: [],
52+
implementation: {
53+
container: {
54+
image: "test-image",
55+
},
56+
},
57+
},
58+
};
59+
60+
const mockHandleImportComponent = vi.fn().mockResolvedValue(undefined);
61+
const mockSetClose = vi.fn();
62+
63+
beforeEach(() => {
64+
vi.clearAllMocks();
65+
});
66+
67+
test("should close dialog when Cancel button is clicked", async () => {
68+
render(
69+
<ComponentDuplicateDialog
70+
existingComponent={mockExistingComponent}
71+
newComponent={mockNewComponent}
72+
newComponentDigest="new-digest-456"
73+
setClose={mockSetClose}
74+
handleImportComponent={mockHandleImportComponent}
75+
/>,
76+
);
77+
78+
// Verify dialog is open
79+
expect(
80+
screen.getByTestId("component-duplicate-dialog-title"),
81+
).toBeInTheDocument();
82+
expect(screen.getByText("Component already exists")).toBeInTheDocument();
83+
84+
// Click Cancel button
85+
const cancelButton = screen.getByTestId(
86+
"duplicate-component-cancel-button",
87+
);
88+
89+
await act(async () => {
90+
fireEvent.click(cancelButton);
91+
});
92+
93+
// Verify setClose was called
94+
expect(mockSetClose).toHaveBeenCalledOnce();
95+
expect(mockHandleImportComponent).not.toHaveBeenCalled();
96+
});
97+
98+
test("should replace existing component when Replace existing button is clicked", async () => {
99+
render(
100+
<ComponentDuplicateDialog
101+
existingComponent={mockExistingComponent}
102+
newComponent={mockNewComponent}
103+
newComponentDigest="new-digest-456"
104+
setClose={mockSetClose}
105+
handleImportComponent={mockHandleImportComponent}
106+
/>,
107+
);
108+
109+
// Verify dialog is open with correct existing component info
110+
const existingNameInput = screen.getAllByRole("textbox")[0];
111+
expect(existingNameInput).toHaveValue("ExistingComponent");
112+
expect(existingNameInput).toHaveAttribute("readonly");
113+
114+
// Click Replace existing button
115+
const replaceButton = screen.getByTestId(
116+
"duplicate-component-replace-existing-button",
117+
);
118+
119+
await act(async () => {
120+
await fireEvent.click(replaceButton);
121+
});
122+
123+
await waitFor(() => {
124+
// Verify deleteComponentFileFromList was called with correct parameters
125+
expect(deleteComponentFileFromList).toHaveBeenCalledWith(
126+
"user_components",
127+
"ExistingComponent",
128+
);
129+
130+
// Verify handleImportComponent was called with the original text
131+
expect(mockHandleImportComponent).toHaveBeenCalledWith(
132+
mockNewComponent.text,
133+
);
134+
135+
// Verify setClose was called
136+
expect(mockSetClose).toHaveBeenCalledOnce();
137+
});
138+
});
139+
140+
test("should import as new component when Import as new button is clicked", async () => {
141+
render(
142+
<ComponentDuplicateDialog
143+
existingComponent={mockExistingComponent}
144+
newComponent={mockNewComponent}
145+
newComponentDigest="new-digest-456"
146+
setClose={mockSetClose}
147+
handleImportComponent={mockHandleImportComponent}
148+
/>,
149+
);
150+
151+
// Find the new component name input (based on the rendering order)
152+
const nameInputs = screen.getAllByRole("textbox");
153+
const newNameInput = nameInputs[2]; // Third input is the editable new component name
154+
155+
// Verify initial state
156+
expect(newNameInput).toHaveValue("NewComponent");
157+
158+
// Change the name to something different
159+
await act(async () => {
160+
await fireEvent.change(newNameInput, {
161+
target: { value: "RenamedComponent" },
162+
});
163+
const importButton = screen.getByText("Import as new");
164+
fireEvent.click(importButton);
165+
});
166+
167+
await waitFor(() => {
168+
// Verify handleImportComponent was called with renamed text
169+
expect(mockHandleImportComponent).toHaveBeenCalledWith(
170+
expect.stringContaining("name: RenamedComponent"),
171+
);
172+
173+
// Verify deleteComponentFileFromList was NOT called
174+
expect(deleteComponentFileFromList).not.toHaveBeenCalled();
175+
176+
// Verify setClose was called
177+
expect(mockSetClose).toHaveBeenCalledOnce();
178+
});
179+
});
180+
181+
test("should disable Import as new button when name is the same as existing", async () => {
182+
render(
183+
<ComponentDuplicateDialog
184+
existingComponent={mockExistingComponent}
185+
newComponent={mockNewComponent}
186+
newComponentDigest="new-digest-456"
187+
setClose={mockSetClose}
188+
handleImportComponent={mockHandleImportComponent}
189+
/>,
190+
);
191+
192+
const importButton = screen.getByText("Import as new");
193+
const nameInputs = screen.getAllByRole("textbox");
194+
const newNameInput = nameInputs[2]; // Third input is the editable new component name
195+
196+
// Initially, the name should be "NewComponent" and button should be enabled
197+
expect(newNameInput).toHaveValue("NewComponent");
198+
expect(importButton).toBeEnabled();
199+
200+
// Change name to be same as existing component - should disable button
201+
await act(async () => {
202+
fireEvent.change(newNameInput, {
203+
target: { value: "ExistingComponent" },
204+
});
205+
});
206+
207+
expect(importButton).toBeDisabled();
208+
});
209+
210+
test("should not render dialog when required props are missing", () => {
211+
// Test with missing newComponent
212+
const { container: container1 } = render(
213+
<ComponentDuplicateDialog
214+
existingComponent={mockExistingComponent}
215+
newComponent={null}
216+
setClose={mockSetClose}
217+
handleImportComponent={mockHandleImportComponent}
218+
/>,
219+
);
220+
expect(container1.querySelector("[role='dialog']")).not.toBeInTheDocument();
221+
222+
// Test with missing existingComponent
223+
const { container: container2 } = render(
224+
<ComponentDuplicateDialog
225+
existingComponent={undefined}
226+
newComponent={mockNewComponent}
227+
setClose={mockSetClose}
228+
handleImportComponent={mockHandleImportComponent}
229+
/>,
230+
);
231+
expect(container2.querySelector("[role='dialog']")).not.toBeInTheDocument();
232+
});
233+
234+
test("should display correct component information in the dialog", () => {
235+
render(
236+
<ComponentDuplicateDialog
237+
existingComponent={mockExistingComponent}
238+
newComponent={mockNewComponent}
239+
newComponentDigest="new-digest-456"
240+
setClose={mockSetClose}
241+
handleImportComponent={mockHandleImportComponent}
242+
/>,
243+
);
244+
245+
// Check dialog title and descriptions
246+
expect(screen.getByText("Component already exists")).toBeInTheDocument();
247+
expect(
248+
screen.getByText(/The component you are trying to import already exists/),
249+
).toBeInTheDocument();
250+
expect(
251+
screen.getByText(/Note: "Replace existing" will use the existing name/),
252+
).toBeInTheDocument();
253+
254+
// Check existing component section
255+
expect(screen.getByText("Existing Component")).toBeInTheDocument();
256+
const textboxes = screen.getAllByRole("textbox");
257+
258+
// Existing component name (first textbox)
259+
expect(textboxes[0]).toHaveValue("ExistingComponent");
260+
expect(textboxes[0]).toHaveAttribute("readonly");
261+
expect(textboxes[0]).toHaveClass("border-blue-200", "bg-blue-100/50");
262+
263+
// Existing component digest (second textbox)
264+
expect(textboxes[1]).toHaveValue("existing-digest-123");
265+
expect(textboxes[1]).toHaveAttribute("readonly");
266+
expect(textboxes[1]).toHaveClass("border-blue-200", "bg-blue-100/50");
267+
268+
// Check new component section
269+
expect(screen.getByText("New Component")).toBeInTheDocument();
270+
271+
// New component name (third textbox) - should be editable
272+
expect(textboxes[2]).toHaveValue("NewComponent");
273+
expect(textboxes[2]).not.toHaveAttribute("readonly");
274+
expect(textboxes[2]).toHaveFocus(); // Has autoFocus
275+
276+
// New component digest (fourth textbox)
277+
expect(textboxes[3]).toHaveValue("new-digest-456");
278+
expect(textboxes[3]).toHaveAttribute("readonly");
279+
});
280+
});

0 commit comments

Comments
 (0)