Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete bound output code if deleting element #213

Merged
merged 17 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions inst/communication-types/src/AppInfo.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import type { ShinyUiNode } from "editor/src/ui-node-definitions/ShinyUiNode";

import type { InputOutputLocations } from "./MessageToBackend";

/**
* What mode is the editor currently in. This will influence what code is
* generated and elements are visible in the elements palette
Expand All @@ -15,7 +13,6 @@ export type AppInfo = {
ui_tree: ShinyUiNode;
app_script: string;
language: LanguageMode;
server_locations?: InputOutputLocations;
app: ScriptGenerationTemplate;
};

Expand Down
30 changes: 5 additions & 25 deletions inst/communication-types/src/MessageToBackend.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { ShinyUiNode } from "editor/src/ui-node-definitions/ShinyUiNode";
import type { getNodePositionAndIndent } from "treesitter-parsers";

import { isRecord } from "./isRecord";
import type { MessageUnion } from "./MessageUnion";
Expand Down Expand Up @@ -66,44 +65,25 @@ export type CompanionEditorPosition = "BESIDE";
/**
* Single location in a script
*/
export type ScriptPosition = {
type ScriptPosition = {
row: number;
column: number;
};
/**
* Range within a script. For something like a function definition etc..
*/
export type ScriptRange = {
start: ScriptPosition;
end: ScriptPosition;
};

/**
* List of ranges within a script where a given set of server code is located
*/
export type ServerPositions = ScriptRange[];

/**
* Key-value store using an object pointing to where in the server code a given input or
* output's references live.
*/
export type ServerLocations = Record<string, ServerPositions>;
export type ServerPositions = {
start: ScriptPosition;
end: ScriptPosition;
}[];

/**
* Key-value store using `Map` pointing to where in the server code a given input or
* output's references live.
*/
export type ServerPositionMap = Map<string, ServerPositions>;

/**
* Locations of inputs and outputs in the server
*/
export type InputOutputLocations = {
input_positions: ServerLocations;
output_positions: ServerLocations;
server_fn: ReturnType<typeof getNodePositionAndIndent>;
};

export type ParsedAppInfo = {
file_lines: string[];
loaded_libraries: string[];
Expand Down

Large diffs are not rendered by default.

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions inst/editor/build/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
Learn how to configure a non-root public URL by running `npm run build`.
-->
<title>Shiny UI Editor</title>
<script type="module" crossorigin src="./assets/index-9ff4babb.js"></script>
<link rel="stylesheet" href="./assets/index-87697c86.css">
<script type="module" crossorigin src="./assets/index-b2d1ab08.js"></script>
<link rel="stylesheet" href="./assets/index-4c252521.css">
</head>

<body>
Expand Down
11 changes: 3 additions & 8 deletions inst/editor/playwright/IdArgumentInput.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { test, expect } from "@playwright/test";
import { expect, test } from "@playwright/test";

import { mockBackendState } from "./utils/mockBackend";
import { startupMockedApp } from "./utils/mockBackend";

const appScript = `library(shiny)
library(gridlayout)
Expand Down Expand Up @@ -56,12 +56,7 @@ shinyApp(ui, server)
test("Switching between two inputs doesn't swap their values", async ({
page,
}) => {
await mockBackendState(page, { app_script: appScript, language: "R" });

await page.goto("/");

// Make sure we get past the loading splash page
await expect(page.getByRole("heading", { name: "Elements" })).toBeVisible();
await startupMockedApp(page, { app_script: appScript, language: "R" });

// Click the "Get app script" button to open the app script modal
await page.click("text=Get app script");
Expand Down
15 changes: 4 additions & 11 deletions inst/editor/playwright/NamedListInput.spec.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { test, expect } from "@playwright/test";
import { expect, test } from "@playwright/test";

import type { ShinyUiNode } from "../src/ui-node-definitions/ShinyUiNode";

import { mockBackendState } from "./utils/mockBackend";
import { startupMockedApp } from "./utils/mockBackend";

const testingUiTree: ShinyUiNode = {
id: "grid_page",
Expand Down Expand Up @@ -52,12 +52,7 @@ const testingUiTree: ShinyUiNode = {
test("Switching between two inputs doesn't swap their values", async ({
page,
}) => {
await mockBackendState(page, { ui_tree: testingUiTree, language: "R" });

await page.goto("/");

// Make sure we get past the loading splash page
await expect(page.getByRole("heading", { name: "Elements" })).toBeVisible();
await startupMockedApp(page, { ui_tree: testingUiTree, language: "R" });

// Select first radio buttons node
await page.getByText("Radio A").click();
Expand Down Expand Up @@ -93,9 +88,7 @@ test("Switching between two inputs doesn't swap their values", async ({
});

test("Can add new element to a named list", async ({ page }) => {
await mockBackendState(page, { ui_tree: testingUiTree, language: "R" });

await page.goto("/");
await startupMockedApp(page, { ui_tree: testingUiTree, language: "R" });

const propertiesPanel = page.getByLabel(/properties panel/i);

Expand Down
181 changes: 181 additions & 0 deletions inst/editor/playwright/deleteOutputServerBindings.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
import type { Page } from "@playwright/test";
import { expect, test } from "@playwright/test";

import { startupMockedApp } from "./utils/mockBackend";

const appScript = `library(shiny)
library(gridlayout)
library(bslib)
library(DT)

ui <- grid_page(
layout = c(
"sidebar",
"A ",
"B "
),
gap_size = "1rem",
col_sizes = c(
"1fr"
),
row_sizes = c(
"1fr",
"1fr",
"1fr"
),
grid_card(
area = "sidebar",
card_body(
numericInput(
inputId = "numRows",
label = "Number of table rows",
value = 10,
min = 1,
step = 1,
width = "100%"
)
)
),
grid_card(
area = "A",
card_body(DTOutput(outputId = "myTable", width = "100%"))
),
grid_card(
area = "B",
card_body(DTOutput(outputId = "unboundTable", width = "100%"))
)
)

server <- function(input, output) {

output$myTable <- renderDT({
head(faithful, input$numRows)
})

output$myTable2 <- renderDT({
head(faithful, input$numRows1)
})
}

shinyApp(ui, server)
`;

test("Deleting an output with a bound render function in the server will ask for confirmation", async ({
page,
}) => {
await startupMockedApp(page, { app_script: appScript, language: "R" });

await openAppScriptModal(page);

// Make sure that the line `output$myTable <- renderDT({` is visible in the modal's code chunk
await expect(page.getByLabel(/app script/i)).toContainText(
"output$myTable <- renderDT"
);

// Make sure that the line `head(faithful, input$numRows)` is visible in the modal's code chunk
await expect(page.getByLabel(/app script/i)).toContainText(
"head(faithful, input$numRows)"
);

// Close modal
await closeAppScriptModal(page);

// Click on the table output node to select it
await page.getByText("myTable").click();

// Click the "Delete" button
await page.getByRole("button", { name: "Delete Element" }).click();

// Verify that we now have a popup asking if we want to delete the server code as well
await expect(page.getByText(/element has server code/i)).toBeVisible();

// Make sure that this hasnt deleted the table element itself yet
await expect(page.getByText("myTable")).toBeVisible();

// Clicking the delete button for both should... delete both
await page.getByRole("button", { name: "Element & Server Code" }).click();

// Make sure that the table is no longer visible
await expect(page.getByText("myTable")).not.toBeVisible();

// Make sure that the line `output$myTable <- renderDT({` is no longer visible in the modal's code chunk
// Click the "Get app script" button to open the app script modal
await openAppScriptModal(page);

await expect(page.getByLabel(/app script/i)).not.toContainText(
"output$myTable <- renderDT"
);

// Make sure the similarly named output$myTable2 is still there
await expect(page.getByLabel(/app script/i)).toContainText(
"output$myTable2 <- renderDT"
);
});

test("User can choose to keep server code when deleting bound output", async ({
page,
}) => {
await startupMockedApp(page, { app_script: appScript, language: "R" });

await openAppScriptModal(page);

// Wait for the modal to open
await page.getByRole("dialog").isVisible();

// Make sure that the line `output$myTable <- renderDT({` is visible in the modal's code chunk
await expect(page.getByLabel(/app script/i)).toContainText(
"output$myTable <- renderDT"
);

// Make sure that the line `head(faithful, input$numRows)` is visible in the modal's code chunk
await expect(page.getByLabel(/app script/i)).toContainText(
"head(faithful, input$numRows)"
);

await closeAppScriptModal(page);

// Click on the table output node to select it
await page.getByText("myTable").click();

// Click the "Delete" button
await page.getByRole("button", { name: "Delete Element" }).click();

// Clicking the delete button for just the node should only delete the node but leave the server code
await page.getByRole("button", { name: "Element Only" }).click();

// Make sure that the table is no longer visible
await expect(page.getByText("myTable")).not.toBeVisible();

// Make sure that the line `output$myTable <- renderDT({` is still visible in the modal's code chunk
await openAppScriptModal(page);

await expect(page.getByLabel(/app script/i)).toContainText(
"output$myTable <- renderDT"
);
});

test("Outputs that don't have any bound server code just get deleted without a popup", async ({
page,
}) => {
await startupMockedApp(page, { app_script: appScript, language: "R" });

await page.getByText("unboundTable").click();

await page.getByRole("button", { name: "Delete Element" }).click();

// Make sure that the table is no longer visible
await expect(page.getByText("unboundTable")).not.toBeVisible();
});

async function openAppScriptModal(page: Page) {
// Click the "Get app script" button to open the app script modal
await page.click("text=Get app script");

// Wait for the modal to open
await expect(page.getByRole("dialog")).toBeVisible();
}

async function closeAppScriptModal(page: Page) {
// Close modal
await page.click("text=Okay");
}
8 changes: 3 additions & 5 deletions inst/editor/playwright/drag-and-drop.spec.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { test, expect } from "@playwright/test";
import { expect, test } from "@playwright/test";

import type { ShinyUiNode } from "../src/ui-node-definitions/ShinyUiNode";

import { mockBackendState } from "./utils/mockBackend";
import { startupMockedApp } from "./utils/mockBackend";
const testingUiTree: ShinyUiNode = {
id: "grid_page",
namedArgs: {
Expand All @@ -15,9 +15,7 @@ const testingUiTree: ShinyUiNode = {
};

test("Drag and drop an item onto the grid and name area", async ({ page }) => {
await mockBackendState(page, { ui_tree: testingUiTree, language: "R" });

await page.goto("/");
await startupMockedApp(page, { ui_tree: testingUiTree, language: "R" });

// Drag and drop a numeric input onto the upper-left grid cell
await page.dragAndDrop("text=/^Action Button$/", `[data-cell-pos="1-1"]`);
Expand Down
6 changes: 2 additions & 4 deletions inst/editor/playwright/error-boundaries.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { expect, test } from "@playwright/test";

import { errorTestingTree } from "../src/ui-node-definitions/sample_ui_trees/errorTesting";

import { mockBackendState } from "./utils/mockBackend";
import { startupMockedApp } from "./utils/mockBackend";

test("Errors are caught and not allowed to propigate up beyond their local position in app", async ({
page,
Expand All @@ -26,9 +26,7 @@ test("Errors are caught and not allowed to propigate up beyond their local posit
};
});

await mockBackendState(page, { ui_tree: errorTestingTree, language: "R" });

await page.goto("/");
await startupMockedApp(page, { ui_tree: errorTestingTree, language: "R" });

const non_errored_node_selector = page.getByRole("heading", {
name: "Error Node! I throw errors",
Expand Down
Loading
Loading