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

1.18 ports8 #7233

Merged
merged 4 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion designer/client/progressBar.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function getElapsed() {

function getBar(percentage) {
const { barLength, almostDone } = options;
const bar = padEnd(repeat("◼", Math.ceil(percentage * barLength)), barLength, "");
const bar = padEnd(repeat("◼", Math.ceil(percentage * barLength)), barLength, "_");
const barColor = percentage > almostDone ? chalk.green : percentage > (almostDone * 2) / 3 ? chalk.yellow : chalk.red;
return barColor(bar);
}
Expand Down
2 changes: 0 additions & 2 deletions designer/client/src/actions/actionTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ export type ActionTypes =
| "DELETE_NODES"
| "NODES_CONNECTED"
| "NODES_DISCONNECTED"
| "NODE_ADDED"
| "NODES_WITH_EDGES_ADDED"
| "VALIDATION_RESULT"
| "COPY_SELECTION"
| "CUT_SELECTION"
Expand Down
43 changes: 30 additions & 13 deletions designer/client/src/actions/nk/node.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { Dictionary } from "lodash";
import { flushSync } from "react-dom";
import NodeUtils from "../../components/graph/NodeUtils";
import { batchGroupBy } from "../../reducers/graph/batchGroupBy";
import { prepareNewNodesWithLayout } from "../../reducers/graph/utils";
import { getScenarioGraph } from "../../reducers/selectors/graph";
import { getProcessDefinitionData } from "../../reducers/selectors/settings";
import { Edge, EdgeType, NodeId, NodeType, ProcessDefinitionData, ValidationResult } from "../../types";
import { ThunkAction } from "../reduxTypes";
import { layoutChanged, Position } from "./ui/layout";
import { EditNodeAction, EditScenarioLabels } from "./editNode";
import { getProcessDefinitionData } from "../../reducers/selectors/settings";
import { batchGroupBy } from "../../reducers/graph/batchGroupBy";
import NodeUtils from "../../components/graph/NodeUtils";
import { getScenarioGraph } from "../../reducers/selectors/graph";
import { flushSync } from "react-dom";
import { layoutChanged, NodePosition, Position } from "./ui/layout";

export type NodesWithPositions = { node: NodeType; position: Position }[];

Expand All @@ -31,7 +33,9 @@ type NodesDisonnectedAction = {

type NodesWithEdgesAddedAction = {
type: "NODES_WITH_EDGES_ADDED";
nodesWithPositions: NodesWithPositions;
nodes: NodeType[];
layout: NodePosition[];
idMapping: Dictionary<string>;
edges: Edge[];
processDefinitionData: ProcessDefinitionData;
};
Expand All @@ -43,8 +47,8 @@ type ValidationResultAction = {

type NodeAddedAction = {
type: "NODE_ADDED";
node: NodeType;
position: Position;
nodes: NodeType[];
layout: NodePosition[];
};

export function deleteNodes(ids: NodeId[]): ThunkAction {
Expand Down Expand Up @@ -118,13 +122,20 @@ export function injectNode(from: NodeType, middle: NodeType, to: NodeType, { edg
}

export function nodeAdded(node: NodeType, position: Position): ThunkAction {
return (dispatch) => {
return (dispatch, getState) => {
batchGroupBy.startOrContinue();

// We need to disable automatic React batching https://react.dev/blog/2022/03/29/react-v18#new-feature-automatic-batching
// since it breaks redux undo in this case
flushSync(() => {
dispatch({ type: "NODE_ADDED", node, position });
const scenarioGraph = getScenarioGraph(getState());
const { nodes, layout } = prepareNewNodesWithLayout(scenarioGraph.nodes, [{ node, position }], false);

dispatch({
type: "NODE_ADDED",
nodes,
layout,
});
dispatch(layoutChanged());
});
batchGroupBy.end();
Expand All @@ -133,11 +144,17 @@ export function nodeAdded(node: NodeType, position: Position): ThunkAction {

export function nodesWithEdgesAdded(nodesWithPositions: NodesWithPositions, edges: Edge[]): ThunkAction {
return (dispatch, getState) => {
const processDefinitionData = getProcessDefinitionData(getState());
const state = getState();
const processDefinitionData = getProcessDefinitionData(state);
const scenarioGraph = getScenarioGraph(state);
const { nodes, layout, idMapping } = prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, true);

batchGroupBy.startOrContinue();
dispatch({
type: "NODES_WITH_EDGES_ADDED",
nodesWithPositions,
nodes,
layout,
idMapping,
edges,
processDefinitionData,
});
Expand Down
21 changes: 17 additions & 4 deletions designer/client/src/common/ClipboardUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,26 @@ export async function readText(event?: Event): Promise<string> {
}
}

interface WriteText {
(text: string): Promise<string>;
}

// We could have used navigator.clipboard.writeText but it is not defined for
// content delivered via HTTP. The workaround is to create a hidden element
// and then write text into it. After that copy command is used to replace
// clipboard's contents with given text. What is more the hidden element is
// assigned with given id to be able to differentiate between artificial
// copy event and the real one triggered by user.
// Based on https://techoverflow.net/2018/03/30/copying-strings-to-the-clipboard-using-pure-javascript/

export function writeText(text: string): Promise<string> {
const fallbackWriteText: WriteText = (text) => {
return new Promise((resolve) => {
const el = document.createElement("textarea");
el.value = text;
el.setAttribute("readonly", "");
el.className = css({ position: "absolute", left: "-9999px" });
el.className = css({
position: "absolute",
left: "-9999px",
});
el.oncopy = (e) => {
// Skip event triggered by writing selection to the clipboard.
e.stopPropagation();
Expand All @@ -41,4 +47,11 @@ export function writeText(text: string): Promise<string> {
document.execCommand("copy");
document.body.removeChild(el);
});
}
};

export const writeText: WriteText = (text) => {
if (navigator.clipboard?.writeText) {
return navigator.clipboard.writeText(text).then(() => text);
}
return fallbackWriteText(text);
};
Comment on lines +52 to +57
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Add browser compatibility tests for clipboard functionality

The implementation has proper feature detection and fallback handling, but there are no dedicated tests found for the clipboard functionality. While the code structure is sound, consider:

  • Adding unit tests to verify the feature detection logic
  • Including tests for both modern and fallback clipboard implementations
  • Testing error scenarios in both implementations
🔗 Analysis chain

Verify browser compatibility testing

The implementation looks good with proper feature detection and fallback handling. However, ensure that browser compatibility testing is in place.


Let me check the babel configuration to understand the browser compatibility settings.


Let me check for any browser compatibility configurations in package.json as browserslist is commonly defined there.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for browser compatibility testing setup

# Look for browser compatibility tests
rg -l "writeText.*test" --type ts
rg -l "clipboard.*test" --type ts

# Check for browser compatibility configuration
fd -e json -e js "browserslist|babel\.config"

Length of output: 153


Script:

#!/bin/bash
# Check babel configuration for browser targets
cat designer/client/babel.config.json

# Look for any test files related to clipboard functionality
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx . designer/client/src/common/

Length of output: 1667


Script:

#!/bin/bash
# Check package.json for browserslist configuration
cat designer/client/package.json | jq '.browserslist'

# Also check for any clipboard-related test files in the entire project
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx . | rg -i "clipboard"

Length of output: 129

Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { min } from "lodash";
import React, {
createContext,
PropsWithChildren,
Expand All @@ -11,6 +12,7 @@ import React, {
} from "react";
import { useTranslation } from "react-i18next";
import { useDispatch, useSelector } from "react-redux";
import { ActionCreators as UndoActionCreators } from "redux-undo";
import { useDebouncedCallback } from "use-debounce";
import {
copySelection,
Expand All @@ -23,17 +25,15 @@ import {
selectAll,
} from "../../actions/nk";
import { error, success } from "../../actions/notificationActions";
import { ActionCreators as UndoActionCreators } from "redux-undo";
import * as ClipboardUtils from "../../common/ClipboardUtils";
import { tryParseOrNull } from "../../common/JsonUtils";
import { isInputEvent } from "../../containers/BindKeyboardShortcuts";
import { useInterval } from "../../containers/Interval";
import { useDocumentListeners } from "../../containers/useDocumentListeners";
import { canModifySelectedNodes, getSelection, getSelectionState } from "../../reducers/selectors/graph";
import { getCapabilities } from "../../reducers/selectors/other";
import { getProcessDefinitionData } from "../../reducers/selectors/settings";
import NodeUtils from "./NodeUtils";
import { min } from "lodash";
import { useInterval } from "../../containers/Interval";

const hasTextSelection = () => !!window.getSelection().toString();

Expand Down
8 changes: 7 additions & 1 deletion designer/client/src/components/themed/InputWithIcon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,13 @@ export const InputWithIcon = forwardRef<Focusable, Props>(function InputWithIcon
<ThemedInput ref={ref} {...props} />
<div className={addonWrapperStyles}>
{!!props.value && onClear && (
<div className={addonStyles} onClick={onClear}>
<div
className={addonStyles}
onClick={() => {
onClear();
focus({ preventScroll: true });
}}
>
<ClearIcon />
</div>
)}
Expand Down
19 changes: 12 additions & 7 deletions designer/client/src/containers/BindKeyboardShortcuts.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { useCallback, useMemo } from "react";
import { useSelectionActions } from "../components/graph/SelectionContextProvider";
import { EventTrackingSelector, EventTrackingType, TrackEventParams, useEventTracking } from "./event-tracking";
import { useDocumentListeners } from "./useDocumentListeners";
import { EventTrackingSelector, TrackEventParams, EventTrackingType, useEventTracking } from "./event-tracking";

export const isInputTarget = (target: EventTarget): boolean => ["INPUT", "SELECT", "TEXTAREA"].includes(target?.["tagName"]);
export const isInputEvent = (event: Event): boolean => isInputTarget(event?.target);
Expand Down Expand Up @@ -46,12 +46,17 @@ export function BindKeyboardShortcuts({ disabled }: { disabled?: boolean }): JSX
if (isInputEvent(event) || !keyHandler) return;
return keyHandler(event);
},
copy: (event) =>
userActions.copy ? eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, userActions.copy(event)) : null,
paste: (event) =>
userActions.paste ? eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, userActions.paste(event)) : null,
cut: (event) =>
userActions.cut ? eventWithStatistics({ selector: EventTrackingSelector.CutNode }, userActions.cut(event)) : null,
copy: (event) => {
if (isInputEvent(event)) return;
userActions.copy ? eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, userActions.copy(event)) : null;
},
paste: (event) => {
userActions.paste ? eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, userActions.paste(event)) : null;
},
cut: (event) => {
if (isInputEvent(event)) return;
userActions.cut ? eventWithStatistics({ selector: EventTrackingSelector.CutNode }, userActions.cut(event)) : null;
},
Comment on lines +49 to +59
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Several improvements needed in clipboard event handlers

  1. The paste handler is missing an input event check, unlike copy and cut.
  2. The conditional expressions can be simplified.
  3. Consider preventing default behavior to avoid duplicate clipboard operations.

Here's a suggested improvement:

            copy: (event) => {
                if (isInputEvent(event)) return;
-               userActions.copy ? eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, userActions.copy(event)) : null;
+               event.preventDefault();
+               userActions.copy?.(event) && eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, true);
            },
            paste: (event) => {
+               if (isInputEvent(event)) return;
-               userActions.paste ? eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, userActions.paste(event)) : null;
+               event.preventDefault();
+               userActions.paste?.(event) && eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, true);
            },
            cut: (event) => {
                if (isInputEvent(event)) return;
-               userActions.cut ? eventWithStatistics({ selector: EventTrackingSelector.CutNode }, userActions.cut(event)) : null;
+               event.preventDefault();
+               userActions.cut?.(event) && eventWithStatistics({ selector: EventTrackingSelector.CutNode }, true);
            },

This refactor:

  • Adds input event check for paste operations
  • Simplifies the conditional logic using optional chaining
  • Prevents default browser behavior
  • Makes the event tracking more consistent
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
copy: (event) => {
if (isInputEvent(event)) return;
userActions.copy ? eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, userActions.copy(event)) : null;
},
paste: (event) => {
userActions.paste ? eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, userActions.paste(event)) : null;
},
cut: (event) => {
if (isInputEvent(event)) return;
userActions.cut ? eventWithStatistics({ selector: EventTrackingSelector.CutNode }, userActions.cut(event)) : null;
},
copy: (event) => {
if (isInputEvent(event)) return;
event.preventDefault();
userActions.copy?.(event) && eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, true);
},
paste: (event) => {
if (isInputEvent(event)) return;
event.preventDefault();
userActions.paste?.(event) && eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, true);
},
cut: (event) => {
if (isInputEvent(event)) return;
event.preventDefault();
userActions.cut?.(event) && eventWithStatistics({ selector: EventTrackingSelector.CutNode }, true);
},

}),
[eventWithStatistics, keyHandlers, userActions],
);
Expand Down
110 changes: 110 additions & 0 deletions designer/client/src/reducers/graph/__snapshots__/utils.test.ts.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`GraphUtils prepareNewNodesWithLayout should update union output expression parameter with an updated node name when new unique node ids created 1`] = `
{
"idMapping": {
"union": "union (copy 1)",
"variable 1": "variable 1 (copy 1)",
"variable 2": "variable 2 (copy 1)",
},
"layout": [
{
"id": "variable 1 (copy 1)",
"position": {
"x": 350,
"y": 859,
},
},
{
"id": "variable 2 (copy 1)",
"position": {
"x": 710,
"y": 859,
},
},
{
"id": "union (copy 1)",
"position": {
"x": 530,
"y": 1039,
},
},
],
"nodes": [
{
"additionalFields": {
"description": null,
"layoutData": {
"x": 0,
"y": 720,
},
},
"branchParameters": undefined,
"id": "variable 1 (copy 1)",
"type": "Variable",
"value": {
"expression": "'value'",
"language": "spel",
},
"varName": "varName1",
},
{
"additionalFields": {
"description": null,
"layoutData": {
"x": 360,
"y": 720,
},
},
"branchParameters": undefined,
"id": "variable 2 (copy 1)",
"type": "Variable",
"value": {
"expression": "'value'",
"language": "spel",
},
"varName": "varName2",
},
{
"additionalFields": {
"description": null,
"layoutData": {
"x": 180,
"y": 900,
},
},
"branchParameters": [
{
"branchId": "variable 1 (copy 1)",
"parameters": [
{
"expression": {
"expression": "1",
"language": "spel",
},
"name": "Output expression",
},
],
},
{
"branchId": "variable 2 (copy 1)",
"parameters": [
{
"expression": {
"expression": "2",
"language": "spel",
},
"name": "Output expression",
},
],
},
],
"id": "union (copy 1)",
"nodeType": "union",
"outputVar": "outputVar",
"parameters": [],
"type": "Join",
},
],
}
`;
Loading
Loading