Unverified 提交 fd5c9ee1 authored 作者: Will Chen's avatar Will Chen 提交者: GitHub

Fix editor file switch race (#3168)

<!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/dyad-sh/dyad/pull/3168" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> --------- Co-authored-by: 's avatarClaude <noreply@anthropic.com> Co-authored-by: 's avatarclaude[bot] <41898282+claude[bot]@users.noreply.github.com>
上级 49d415dd
import { test } from "./helpers/test_helper"; import { test, Timeout } from "./helpers/test_helper";
import { expect } from "@playwright/test"; import { expect, type Page } from "@playwright/test";
import fs from "fs"; import fs from "fs";
import path from "path"; import path from "path";
async function getActiveEditorModelPath(page: Page): Promise<string | null> {
return page.evaluate(() => {
// Monaco attaches itself to the window in the packaged app.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const monaco = (window as any).monaco;
if (!monaco) {
return null;
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const editor = monaco.editor.getEditors().find((candidate: any) => {
return candidate.getModel();
});
const model = editor?.getModel();
return model?.uri?.path ?? null;
});
}
async function selectFileAndWaitForEditor(page: Page, fileName: string) {
await page.getByText(fileName, { exact: true }).click();
await expect(async () => {
const modelPath = await getActiveEditorModelPath(page);
expect(modelPath).toContain(fileName);
}).toPass({ timeout: Timeout.MEDIUM });
}
async function replaceEditorContent(page: Page, content: string) {
const editorContent = page.getByRole("textbox", {
name: "Editor content",
});
await expect(editorContent).toBeVisible();
await editorContent.click({ force: true });
await page.keyboard.press("ControlOrMeta+a");
await page.keyboard.type(content);
}
test("edit code", async ({ po }) => { test("edit code", async ({ po }) => {
await po.setUp({ autoApprove: true }); await po.setUp({ autoApprove: true });
const editedFilePath = path.join("src", "components", "made-with-dyad.tsx"); const editedFilePath = path.join("src", "components", "made-with-dyad.tsx");
...@@ -12,16 +48,14 @@ test("edit code", async ({ po }) => { ...@@ -12,16 +48,14 @@ test("edit code", async ({ po }) => {
await po.previewPanel.clickTogglePreviewPanel(); await po.previewPanel.clickTogglePreviewPanel();
await po.previewPanel.selectPreviewMode("code"); await po.previewPanel.selectPreviewMode("code");
await po.page.getByText("made-with-dyad.tsx").click(); await expect(
// Wait for the editor to load and then fill in the new content po.page.getByText("Loading files...", { exact: false }),
const editorContent = po.page.getByRole("textbox", { ).toBeHidden({
name: "Editor content", timeout: Timeout.LONG,
}); });
await expect(editorContent).toBeVisible();
// Monaco editor intercepts pointer events, so we need to use force: true await selectFileAndWaitForEditor(po.page, "made-with-dyad.tsx");
await editorContent.click({ force: true }); await replaceEditorContent(po.page, "export const MadeWithDyad = ;");
await po.page.keyboard.press("ControlOrMeta+a");
await po.page.keyboard.type("export const MadeWithDyad = ;");
// Save the file // Save the file
await po.page.getByTestId("save-file-button").click(); await po.page.getByTestId("save-file-button").click();
...@@ -38,49 +72,48 @@ test("edit code", async ({ po }) => { ...@@ -38,49 +72,48 @@ test("edit code", async ({ po }) => {
expect(editedFile).toContain("export const MadeWithDyad = ;"); expect(editedFile).toContain("export const MadeWithDyad = ;");
}); });
test("edit code edits the right file", async ({ po }) => { test("edit code edits the right file during rapid switches", async ({ po }) => {
await po.setUp({ autoApprove: true }); await po.setUp({ autoApprove: true });
const editedFilePath = path.join("src", "components", "made-with-dyad.tsx"); const firstOpenedFilePath = path.join(
"src",
"components",
"made-with-dyad.tsx",
);
const robotsFilePath = path.join("public", "robots.txt"); const robotsFilePath = path.join("public", "robots.txt");
await po.sendPrompt("foo"); await po.sendPrompt("foo");
const appPath = await po.appManagement.getCurrentAppPath(); const appPath = await po.appManagement.getCurrentAppPath();
const originalRobotsFile = fs.readFileSync( let firstFileEdit = "";
path.join(appPath, robotsFilePath), let updatedRobotsFile = "";
"utf8",
);
await po.previewPanel.clickTogglePreviewPanel(); await po.previewPanel.clickTogglePreviewPanel();
await po.previewPanel.selectPreviewMode("code"); await po.previewPanel.selectPreviewMode("code");
await po.page.getByText("made-with-dyad.tsx").click(); await expect(
// Wait for the editor to load and then fill in the new content po.page.getByText("Loading files...", { exact: false }),
const editorContent = po.page.getByRole("textbox", { ).toBeHidden({
name: "Editor content", timeout: Timeout.LONG,
}); });
await expect(editorContent).toBeVisible();
// Monaco editor intercepts pointer events, so we need to use force: true
await editorContent.click({ force: true });
await po.page.keyboard.press("ControlOrMeta+a");
await po.page.keyboard.type("export const MadeWithDyad = ;");
// Save the file by switching files await selectFileAndWaitForEditor(po.page, "made-with-dyad.tsx");
await po.page.getByText("robots.txt").click(); for (const round of [1, 2, 3]) {
firstFileEdit = `export const MadeWithDyad = "round-${round}";\n`;
updatedRobotsFile = `User-agent: *\nDisallow: /round-${round}\n`;
// Expect toast to be visible await replaceEditorContent(po.page, firstFileEdit);
await expect(po.page.getByText("File saved")).toBeVisible(); await selectFileAndWaitForEditor(po.page, "robots.txt");
await replaceEditorContent(po.page, updatedRobotsFile);
// We are NOT snapshotting the app files because the Monaco UI edit await selectFileAndWaitForEditor(po.page, "made-with-dyad.tsx");
// is not deterministic. }
const editedFile = fs.readFileSync(
path.join(appPath, editedFilePath),
"utf8",
);
expect(editedFile).toContain("export const MadeWithDyad = ;");
// Make sure the robots.txt file is not edited await expect
const editedRobotsFile = fs.readFileSync( .poll(
path.join(appPath, robotsFilePath), () => fs.readFileSync(path.join(appPath, firstOpenedFilePath), "utf8"),
"utf8", { timeout: Timeout.MEDIUM },
); )
expect(editedRobotsFile).toEqual(originalRobotsFile); .toEqual(firstFileEdit);
await expect
.poll(() => fs.readFileSync(path.join(appPath, robotsFilePath), "utf8"), {
timeout: Timeout.MEDIUM,
})
.toEqual(updatedRobotsFile);
}); });
...@@ -116,6 +116,8 @@ If this happens: ...@@ -116,6 +116,8 @@ If this happens:
- **Keyboard navigation events (ArrowUp/ArrowDown)**: Add `await page.waitForTimeout(100)` between sequential keyboard presses to let the UI state settle. Rapid keypresses can cause race conditions in menu navigation. - **Keyboard navigation events (ArrowUp/ArrowDown)**: Add `await page.waitForTimeout(100)` between sequential keyboard presses to let the UI state settle. Rapid keypresses can cause race conditions in menu navigation.
- **Navigation to tabs**: Use `await expect(link).toBeVisible({ timeout: Timeout.EXTRA_LONG })` before clicking tab links (especially in `goToAppsTab()`). Electron sidebar links can take time to render during app initialization. - **Navigation to tabs**: Use `await expect(link).toBeVisible({ timeout: Timeout.EXTRA_LONG })` before clicking tab links (especially in `goToAppsTab()`). Electron sidebar links can take time to render during app initialization.
- **Confirming flakiness**: Use `PLAYWRIGHT_RETRIES=0 PLAYWRIGHT_HTML_OPEN=never npm run e2e -- e2e-tests/<spec> --repeat-each=10` to reproduce flaky tests. `PLAYWRIGHT_RETRIES=0` is critical — CI defaults to 2 retries, hiding flakiness. - **Confirming flakiness**: Use `PLAYWRIGHT_RETRIES=0 PLAYWRIGHT_HTML_OPEN=never npm run e2e -- e2e-tests/<spec> --repeat-each=10` to reproduce flaky tests. `PLAYWRIGHT_RETRIES=0` is critical — CI defaults to 2 retries, hiding flakiness.
- **Monaco file-switch assertions**: For code-editor tests, don't stop at waiting for the editor textbox to appear. Wait until Monaco's active model URI matches the file you clicked; otherwise the test can type into a still-switching editor model and miss real file-switch races.
- **Monaco race repros**: If a file-editor bug only appears during quick tab/file changes, alternate between the affected files several times in one test before declaring it non-reproducible. A single switch often misses save-vs-switch timing bugs that show up immediately under `--repeat-each`.
## Real Socket Firewall E2E tests ## Real Socket Firewall E2E tests
......
...@@ -113,6 +113,7 @@ export const CodeView = ({ loading, app }: CodeViewProps) => { ...@@ -113,6 +113,7 @@ export const CodeView = ({ loading, app }: CodeViewProps) => {
<div className="w-2/3"> <div className="w-2/3">
{selectedFile ? ( {selectedFile ? (
<FileEditor <FileEditor
key={`${app.id ?? "unknown"}:${selectedFile.path}`}
appId={app.id ?? null} appId={app.id ?? null}
filePath={selectedFile.path} filePath={selectedFile.path}
initialLine={selectedFile.line ?? null} initialLine={selectedFile.line ?? null}
......
...@@ -18,6 +18,7 @@ import { ...@@ -18,6 +18,7 @@ import {
TooltipContent, TooltipContent,
} from "@/components/ui/tooltip"; } from "@/components/ui/tooltip";
import { useTranslation } from "react-i18next"; import { useTranslation } from "react-i18next";
import { enqueueFileSave, getFileSaveQueueKey } from "./fileSaveQueue";
interface FileEditorProps { interface FileEditorProps {
appId: number | null; appId: number | null;
...@@ -112,26 +113,36 @@ export const FileEditor = ({ ...@@ -112,26 +113,36 @@ export const FileEditor = ({
const isSavingRef = useRef<boolean>(false); const isSavingRef = useRef<boolean>(false);
const needsSaveRef = useRef<boolean>(false); const needsSaveRef = useRef<boolean>(false);
const currentValueRef = useRef<string | undefined>(undefined); const currentValueRef = useRef<string | undefined>(undefined);
const hasInitializedContentRef = useRef(false);
const isMountedRef = useRef(false);
const queryClient = useQueryClient(); const queryClient = useQueryClient();
const { checkProblems } = useCheckProblems(appId); const { checkProblems } = useCheckProblems(appId);
// Update state when content loads
useEffect(() => { useEffect(() => {
if (content !== null) { isMountedRef.current = true;
setValue(content); return () => {
originalValueRef.current = content; isMountedRef.current = false;
currentValueRef.current = content; };
needsSaveRef.current = false; }, []);
setDisplayUnsavedChanges(false);
setIsSaving(false);
}
}, [content, filePath]);
// Sync the UI with the needsSave ref // Initialize editor state from disk once per mounted file editor instance.
useEffect(() => { useEffect(() => {
setDisplayUnsavedChanges(needsSaveRef.current); if (
}, [needsSaveRef.current]); content === null ||
(hasInitializedContentRef.current && needsSaveRef.current)
) {
return;
}
hasInitializedContentRef.current = true;
setValue(content);
originalValueRef.current = content;
currentValueRef.current = content;
needsSaveRef.current = false;
setDisplayUnsavedChanges(false);
setIsSaving(false);
}, [content]);
// Determine if dark mode based on theme // Determine if dark mode based on theme
const isDarkMode = const isDarkMode =
...@@ -168,9 +179,8 @@ export const FileEditor = ({ ...@@ -168,9 +179,8 @@ export const FileEditor = ({
navigateToLine(initialLine); navigateToLine(initialLine);
} }
// Listen for model content change events // Save when the editor loses focus and the current model is dirty.
editor.onDidBlurEditorText(() => { editor.onDidBlurEditorText(() => {
console.log("Editor text blurred, checking if save needed");
if (needsSaveRef.current) { if (needsSaveRef.current) {
saveFile(); saveFile();
} }
...@@ -190,24 +200,40 @@ export const FileEditor = ({ ...@@ -190,24 +200,40 @@ export const FileEditor = ({
// Save the file // Save the file
const saveFile = async () => { const saveFile = async () => {
if ( if (
!appId || appId === null ||
!currentValueRef.current || currentValueRef.current === undefined ||
!needsSaveRef.current || !needsSaveRef.current ||
isSavingRef.current isSavingRef.current
) )
return; return;
const saveAppId = appId;
const saveFilePath = filePath;
const savedValue = currentValueRef.current;
const saveQueueKey = getFileSaveQueueKey(saveAppId, saveFilePath);
const performSave = () =>
ipc.app.editAppFile({
appId: saveAppId,
filePath: saveFilePath,
content: savedValue,
});
try { try {
isSavingRef.current = true; isSavingRef.current = true;
setIsSaving(true); if (isMountedRef.current) {
setIsSaving(true);
}
const { warning } = await ipc.app.editAppFile({ const { warning } = await enqueueFileSave(saveQueueKey, performSave);
appId, queryClient.setQueryData(
filePath, queryKeys.appFiles.content({
content: currentValueRef.current, appId: saveAppId,
}); filePath: saveFilePath,
}),
savedValue,
);
await queryClient.invalidateQueries({ await queryClient.invalidateQueries({
queryKey: queryKeys.versions.list({ appId }), queryKey: queryKeys.versions.list({ appId: saveAppId }),
}); });
if (settings?.enableAutoFixProblems) { if (settings?.enableAutoFixProblems) {
checkProblems(); checkProblems();
...@@ -218,14 +244,19 @@ export const FileEditor = ({ ...@@ -218,14 +244,19 @@ export const FileEditor = ({
showSuccess(t("preview.fileSaved")); showSuccess(t("preview.fileSaved"));
} }
originalValueRef.current = currentValueRef.current; originalValueRef.current = savedValue;
needsSaveRef.current = false; const hasNewerEdits = currentValueRef.current !== savedValue;
setDisplayUnsavedChanges(false); needsSaveRef.current = hasNewerEdits;
if (isMountedRef.current) {
setDisplayUnsavedChanges(hasNewerEdits);
}
} catch (error) { } catch (error) {
showError(error); showError(error);
} finally { } finally {
isSavingRef.current = false; isSavingRef.current = false;
setIsSaving(false); if (isMountedRef.current) {
setIsSaving(false);
}
} }
}; };
...@@ -247,7 +278,7 @@ export const FileEditor = ({ ...@@ -247,7 +278,7 @@ export const FileEditor = ({
return <div className="p-4 text-red-500">Error: {error.message}</div>; return <div className="p-4 text-red-500">Error: {error.message}</div>;
} }
if (!content) { if (content === null) {
return ( return (
<div className="p-4 text-gray-500">{t("preview.noContentAvailable")}</div> <div className="p-4 text-gray-500">{t("preview.noContentAvailable")}</div>
); );
......
const fileSaveQueueByPath = new Map<string, Promise<void>>();
export function getFileSaveQueueKey(appId: number, filePath: string) {
return `${appId}:${filePath}`;
}
export async function enqueueFileSave<T>(
queueKey: string,
saveOperation: () => Promise<T>,
) {
const previousSave = fileSaveQueueByPath.get(queueKey) ?? Promise.resolve();
const queuedSave = previousSave.catch(() => undefined).then(saveOperation);
const trackedSave = queuedSave.then(
() => undefined,
() => undefined,
);
fileSaveQueueByPath.set(queueKey, trackedSave);
try {
return await queuedSave;
} finally {
if (fileSaveQueueByPath.get(queueKey) === trackedSave) {
fileSaveQueueByPath.delete(queueKey);
}
}
}
Markdown 格式
0%
您添加了 0 到此讨论。请谨慎行事。
请先完成此评论的编辑!
注册 或者 后发表评论