Copilot commented on code in PR #4043:
URL:
https://github.com/apache/incubator-kie-tools/pull/4043#discussion_r4261907978
##########
packages/bpmn-editor/src/propertiesPanel/codeInput/CodeInput.tsx:
##########
@@ -21,53 +21,92 @@ import * as React from "react";
import { useBpmnEditorStore } from "../../store/StoreContext";
import { FormGroup } from "@patternfly/react-core/dist/js/components/Form";
import { Label } from "@patternfly/react-core/dist/js/components/Label";
+import { MenuToggle } from
"@patternfly/react-core/dist/js/components/MenuToggle";
+import { Select, SelectList, SelectOption } from
"@patternfly/react-core/dist/js/components/Select";
import { TextArea } from "@patternfly/react-core/dist/js/components/TextArea";
import "./CodeInput.css";
import { useBpmnEditorI18n } from "../../i18n";
+import { ScriptLanguage } from "../scriptFormats";
export function CodeInput({
label,
- languages,
+ availableLanguages,
+ currentLanguage,
value,
onChange,
+ onLanguageChange,
}: {
label: string;
- languages: string[];
+ availableLanguages: string[];
+ /** Currently selected language. Defaults to the first of `languages`. */
+ currentLanguage?: string;
value: string | undefined;
onChange: (e: React.FormEvent, newCode: string) => void;
+ /** When provided, a dropdown to select the language is shown instead of a
static label. */
+ onLanguageChange?: (newLanguage: ScriptLanguage) => void;
}) {
const { i18n } = useBpmnEditorI18n();
const isReadOnly = useBpmnEditorStore((s) => s.settings.isReadOnly);
+ const [isLanguageDropdownOpen, setIsLanguageDropdownOpen] =
React.useState(false);
- const languageLabel = React.useMemo(() => {
- return (
- <Label
- isCompact={true}
- style={{
- margin: "16px",
- top: "24px",
- right: "0",
- position: "absolute",
- }}
- >
- {languages[0]}
- </Label>
- );
- }, [languages]);
+ const selectedLanguage = currentLanguage ?? availableLanguages[0];
Review Comment:
`CodeInput` mixes `string[]` (`availableLanguages`, `currentLanguage`) with
a `ScriptLanguage`-typed callback, and the doc comment still references
`languages`. This leads to unsafe casting in `onSelect` and allows
`currentLanguage` values that aren't in `availableLanguages`. Recommend
aligning the API types (e.g., make `availableLanguages: ScriptLanguage[]` and
`currentLanguage?: ScriptLanguage`, or make `onLanguageChange` accept
`string`), update the comment to reference `availableLanguages`, and clamp
`selectedLanguage` to the provided options when `currentLanguage` is not
included.
##########
packages/bpmn-editor/src/propertiesPanel/codeInput/CodeInput.tsx:
##########
@@ -21,53 +21,92 @@ import * as React from "react";
import { useBpmnEditorStore } from "../../store/StoreContext";
import { FormGroup } from "@patternfly/react-core/dist/js/components/Form";
import { Label } from "@patternfly/react-core/dist/js/components/Label";
+import { MenuToggle } from
"@patternfly/react-core/dist/js/components/MenuToggle";
+import { Select, SelectList, SelectOption } from
"@patternfly/react-core/dist/js/components/Select";
import { TextArea } from "@patternfly/react-core/dist/js/components/TextArea";
import "./CodeInput.css";
import { useBpmnEditorI18n } from "../../i18n";
+import { ScriptLanguage } from "../scriptFormats";
export function CodeInput({
label,
- languages,
+ availableLanguages,
+ currentLanguage,
value,
onChange,
+ onLanguageChange,
}: {
label: string;
- languages: string[];
+ availableLanguages: string[];
+ /** Currently selected language. Defaults to the first of `languages`. */
+ currentLanguage?: string;
value: string | undefined;
onChange: (e: React.FormEvent, newCode: string) => void;
+ /** When provided, a dropdown to select the language is shown instead of a
static label. */
+ onLanguageChange?: (newLanguage: ScriptLanguage) => void;
Review Comment:
`CodeInput` mixes `string[]` (`availableLanguages`, `currentLanguage`) with
a `ScriptLanguage`-typed callback, and the doc comment still references
`languages`. This leads to unsafe casting in `onSelect` and allows
`currentLanguage` values that aren't in `availableLanguages`. Recommend
aligning the API types (e.g., make `availableLanguages: ScriptLanguage[]` and
`currentLanguage?: ScriptLanguage`, or make `onLanguageChange` accept
`string`), update the comment to reference `availableLanguages`, and clamp
`selectedLanguage` to the provided options when `currentLanguage` is not
included.
##########
packages/bpmn-editor/src/propertiesPanel/codeInput/CodeInput.tsx:
##########
@@ -21,53 +21,92 @@ import * as React from "react";
import { useBpmnEditorStore } from "../../store/StoreContext";
import { FormGroup } from "@patternfly/react-core/dist/js/components/Form";
import { Label } from "@patternfly/react-core/dist/js/components/Label";
+import { MenuToggle } from
"@patternfly/react-core/dist/js/components/MenuToggle";
+import { Select, SelectList, SelectOption } from
"@patternfly/react-core/dist/js/components/Select";
import { TextArea } from "@patternfly/react-core/dist/js/components/TextArea";
import "./CodeInput.css";
import { useBpmnEditorI18n } from "../../i18n";
+import { ScriptLanguage } from "../scriptFormats";
export function CodeInput({
label,
- languages,
+ availableLanguages,
+ currentLanguage,
value,
onChange,
+ onLanguageChange,
}: {
label: string;
- languages: string[];
+ availableLanguages: string[];
+ /** Currently selected language. Defaults to the first of `languages`. */
+ currentLanguage?: string;
value: string | undefined;
onChange: (e: React.FormEvent, newCode: string) => void;
+ /** When provided, a dropdown to select the language is shown instead of a
static label. */
+ onLanguageChange?: (newLanguage: ScriptLanguage) => void;
}) {
const { i18n } = useBpmnEditorI18n();
const isReadOnly = useBpmnEditorStore((s) => s.settings.isReadOnly);
+ const [isLanguageDropdownOpen, setIsLanguageDropdownOpen] =
React.useState(false);
- const languageLabel = React.useMemo(() => {
- return (
- <Label
- isCompact={true}
- style={{
- margin: "16px",
- top: "24px",
- right: "0",
- position: "absolute",
- }}
- >
- {languages[0]}
- </Label>
- );
- }, [languages]);
+ const selectedLanguage = currentLanguage ?? availableLanguages[0];
+
+ const hasLanguageSelector = !!onLanguageChange && availableLanguages.length
> 1;
+ const languageSelectId = `${label}-language-select`;
Review Comment:
Deriving an element `id` from `label` can create invalid/duplicate IDs
(spaces, punctuation, repeated labels), which can break label-to-control
associations. Prefer using `React.useId()` (or another stable unique ID
generator) and keep the label purely for display.
##########
packages/bpmn-editor/src/propertiesPanel/codeInput/CodeInput.tsx:
##########
@@ -21,53 +21,92 @@ import * as React from "react";
import { useBpmnEditorStore } from "../../store/StoreContext";
import { FormGroup } from "@patternfly/react-core/dist/js/components/Form";
import { Label } from "@patternfly/react-core/dist/js/components/Label";
+import { MenuToggle } from
"@patternfly/react-core/dist/js/components/MenuToggle";
+import { Select, SelectList, SelectOption } from
"@patternfly/react-core/dist/js/components/Select";
import { TextArea } from "@patternfly/react-core/dist/js/components/TextArea";
import "./CodeInput.css";
import { useBpmnEditorI18n } from "../../i18n";
+import { ScriptLanguage } from "../scriptFormats";
export function CodeInput({
label,
- languages,
+ availableLanguages,
+ currentLanguage,
value,
onChange,
+ onLanguageChange,
}: {
label: string;
- languages: string[];
+ availableLanguages: string[];
+ /** Currently selected language. Defaults to the first of `languages`. */
+ currentLanguage?: string;
value: string | undefined;
onChange: (e: React.FormEvent, newCode: string) => void;
+ /** When provided, a dropdown to select the language is shown instead of a
static label. */
+ onLanguageChange?: (newLanguage: ScriptLanguage) => void;
}) {
const { i18n } = useBpmnEditorI18n();
const isReadOnly = useBpmnEditorStore((s) => s.settings.isReadOnly);
+ const [isLanguageDropdownOpen, setIsLanguageDropdownOpen] =
React.useState(false);
- const languageLabel = React.useMemo(() => {
- return (
- <Label
- isCompact={true}
- style={{
- margin: "16px",
- top: "24px",
- right: "0",
- position: "absolute",
- }}
- >
- {languages[0]}
- </Label>
- );
- }, [languages]);
+ const selectedLanguage = currentLanguage ?? availableLanguages[0];
+
+ const hasLanguageSelector = !!onLanguageChange && availableLanguages.length
> 1;
+ const languageSelectId = `${label}-language-select`;
return (
- <FormGroup label={`${label}`} style={{ position: "relative" }}>
- <TextArea
- aria-label={`${label}`}
- type={"text"}
- isDisabled={isReadOnly}
- value={value}
- onChange={onChange}
- placeholder={i18n.propertiesPanel.codePlaceholder}
- style={{ resize: "vertical", minHeight: "40px", fontFamily:
"monospace" }}
- rows={3}
- />
- {languageLabel}
- </FormGroup>
+ <>
+ <FormGroup label={`${label}`}>
+ <TextArea
+ aria-label={`${label}`}
+ type={"text"}
+ isDisabled={isReadOnly}
+ value={value}
+ onChange={onChange}
+ placeholder={i18n.propertiesPanel.codePlaceholder}
+ style={{ resize: "vertical", minHeight: "40px", fontFamily:
"monospace" }}
+ rows={3}
+ />
+ {!hasLanguageSelector && (
+ <div style={{ display: "flex", justifyContent: "flex-end",
marginTop: "4px" }}>
+ <Label isCompact={true}>{selectedLanguage}</Label>
+ </div>
+ )}
+ </FormGroup>
+ {hasLanguageSelector && (
+ <FormGroup label={i18n.propertiesPanel.language}
fieldId={languageSelectId}>
+ <Select
+ id={languageSelectId}
Review Comment:
Deriving an element `id` from `label` can create invalid/duplicate IDs
(spaces, punctuation, repeated labels), which can break label-to-control
associations. Prefer using `React.useId()` (or another stable unique ID
generator) and keep the label purely for display.
##########
packages/bpmn-editor/src/propertiesPanel/codeInput/CodeInput.tsx:
##########
@@ -21,53 +21,92 @@ import * as React from "react";
import { useBpmnEditorStore } from "../../store/StoreContext";
import { FormGroup } from "@patternfly/react-core/dist/js/components/Form";
import { Label } from "@patternfly/react-core/dist/js/components/Label";
+import { MenuToggle } from
"@patternfly/react-core/dist/js/components/MenuToggle";
+import { Select, SelectList, SelectOption } from
"@patternfly/react-core/dist/js/components/Select";
import { TextArea } from "@patternfly/react-core/dist/js/components/TextArea";
import "./CodeInput.css";
import { useBpmnEditorI18n } from "../../i18n";
+import { ScriptLanguage } from "../scriptFormats";
export function CodeInput({
label,
- languages,
+ availableLanguages,
+ currentLanguage,
value,
onChange,
+ onLanguageChange,
}: {
label: string;
- languages: string[];
+ availableLanguages: string[];
+ /** Currently selected language. Defaults to the first of `languages`. */
+ currentLanguage?: string;
value: string | undefined;
onChange: (e: React.FormEvent, newCode: string) => void;
+ /** When provided, a dropdown to select the language is shown instead of a
static label. */
+ onLanguageChange?: (newLanguage: ScriptLanguage) => void;
}) {
const { i18n } = useBpmnEditorI18n();
const isReadOnly = useBpmnEditorStore((s) => s.settings.isReadOnly);
+ const [isLanguageDropdownOpen, setIsLanguageDropdownOpen] =
React.useState(false);
- const languageLabel = React.useMemo(() => {
- return (
- <Label
- isCompact={true}
- style={{
- margin: "16px",
- top: "24px",
- right: "0",
- position: "absolute",
- }}
- >
- {languages[0]}
- </Label>
- );
- }, [languages]);
+ const selectedLanguage = currentLanguage ?? availableLanguages[0];
+
+ const hasLanguageSelector = !!onLanguageChange && availableLanguages.length
> 1;
+ const languageSelectId = `${label}-language-select`;
return (
- <FormGroup label={`${label}`} style={{ position: "relative" }}>
- <TextArea
- aria-label={`${label}`}
- type={"text"}
- isDisabled={isReadOnly}
- value={value}
- onChange={onChange}
- placeholder={i18n.propertiesPanel.codePlaceholder}
- style={{ resize: "vertical", minHeight: "40px", fontFamily:
"monospace" }}
- rows={3}
- />
- {languageLabel}
- </FormGroup>
+ <>
+ <FormGroup label={`${label}`}>
+ <TextArea
+ aria-label={`${label}`}
+ type={"text"}
+ isDisabled={isReadOnly}
+ value={value}
+ onChange={onChange}
+ placeholder={i18n.propertiesPanel.codePlaceholder}
+ style={{ resize: "vertical", minHeight: "40px", fontFamily:
"monospace" }}
+ rows={3}
+ />
+ {!hasLanguageSelector && (
+ <div style={{ display: "flex", justifyContent: "flex-end",
marginTop: "4px" }}>
+ <Label isCompact={true}>{selectedLanguage}</Label>
+ </div>
+ )}
+ </FormGroup>
+ {hasLanguageSelector && (
+ <FormGroup label={i18n.propertiesPanel.language}
fieldId={languageSelectId}>
+ <Select
+ id={languageSelectId}
+ isOpen={isLanguageDropdownOpen}
+ selected={selectedLanguage}
+ onOpenChange={setIsLanguageDropdownOpen}
+ onSelect={(_event, selection) => {
+ onLanguageChange(selection as ScriptLanguage);
+ setIsLanguageDropdownOpen(false);
+ }}
Review Comment:
`CodeInput` mixes `string[]` (`availableLanguages`, `currentLanguage`) with
a `ScriptLanguage`-typed callback, and the doc comment still references
`languages`. This leads to unsafe casting in `onSelect` and allows
`currentLanguage` values that aren't in `availableLanguages`. Recommend
aligning the API types (e.g., make `availableLanguages: ScriptLanguage[]` and
`currentLanguage?: ScriptLanguage`, or make `onLanguageChange` accept
`string`), update the comment to reference `availableLanguages`, and clamp
`selectedLanguage` to the provided options when `currentLanguage` is not
included.
##########
packages/bpmn-editor/src/propertiesPanel/codeInput/CodeInput.tsx:
##########
@@ -21,53 +21,92 @@ import * as React from "react";
import { useBpmnEditorStore } from "../../store/StoreContext";
import { FormGroup } from "@patternfly/react-core/dist/js/components/Form";
import { Label } from "@patternfly/react-core/dist/js/components/Label";
+import { MenuToggle } from
"@patternfly/react-core/dist/js/components/MenuToggle";
+import { Select, SelectList, SelectOption } from
"@patternfly/react-core/dist/js/components/Select";
import { TextArea } from "@patternfly/react-core/dist/js/components/TextArea";
import "./CodeInput.css";
import { useBpmnEditorI18n } from "../../i18n";
+import { ScriptLanguage } from "../scriptFormats";
export function CodeInput({
label,
- languages,
+ availableLanguages,
+ currentLanguage,
value,
onChange,
+ onLanguageChange,
}: {
label: string;
- languages: string[];
+ availableLanguages: string[];
+ /** Currently selected language. Defaults to the first of `languages`. */
+ currentLanguage?: string;
value: string | undefined;
onChange: (e: React.FormEvent, newCode: string) => void;
+ /** When provided, a dropdown to select the language is shown instead of a
static label. */
+ onLanguageChange?: (newLanguage: ScriptLanguage) => void;
}) {
const { i18n } = useBpmnEditorI18n();
const isReadOnly = useBpmnEditorStore((s) => s.settings.isReadOnly);
+ const [isLanguageDropdownOpen, setIsLanguageDropdownOpen] =
React.useState(false);
- const languageLabel = React.useMemo(() => {
- return (
- <Label
- isCompact={true}
- style={{
- margin: "16px",
- top: "24px",
- right: "0",
- position: "absolute",
- }}
- >
- {languages[0]}
- </Label>
- );
- }, [languages]);
+ const selectedLanguage = currentLanguage ?? availableLanguages[0];
+
+ const hasLanguageSelector = !!onLanguageChange && availableLanguages.length
> 1;
+ const languageSelectId = `${label}-language-select`;
return (
- <FormGroup label={`${label}`} style={{ position: "relative" }}>
- <TextArea
- aria-label={`${label}`}
- type={"text"}
- isDisabled={isReadOnly}
- value={value}
- onChange={onChange}
- placeholder={i18n.propertiesPanel.codePlaceholder}
- style={{ resize: "vertical", minHeight: "40px", fontFamily:
"monospace" }}
- rows={3}
- />
- {languageLabel}
- </FormGroup>
+ <>
+ <FormGroup label={`${label}`}>
+ <TextArea
+ aria-label={`${label}`}
+ type={"text"}
+ isDisabled={isReadOnly}
+ value={value}
+ onChange={onChange}
+ placeholder={i18n.propertiesPanel.codePlaceholder}
+ style={{ resize: "vertical", minHeight: "40px", fontFamily:
"monospace" }}
+ rows={3}
+ />
+ {!hasLanguageSelector && (
+ <div style={{ display: "flex", justifyContent: "flex-end",
marginTop: "4px" }}>
+ <Label isCompact={true}>{selectedLanguage}</Label>
+ </div>
+ )}
+ </FormGroup>
+ {hasLanguageSelector && (
+ <FormGroup label={i18n.propertiesPanel.language}
fieldId={languageSelectId}>
+ <Select
+ id={languageSelectId}
+ isOpen={isLanguageDropdownOpen}
+ selected={selectedLanguage}
+ onOpenChange={setIsLanguageDropdownOpen}
+ onSelect={(_event, selection) => {
+ onLanguageChange(selection as ScriptLanguage);
+ setIsLanguageDropdownOpen(false);
+ }}
+ popperProps={{ appendTo: document.body, width: "trigger" }}
Review Comment:
Directly referencing `document.body` can break in SSR environments and some
test runners where `document` is undefined. Consider guarding (e.g., only
setting `appendTo` when `typeof document !== \"undefined\"`) or using
PatternFly defaults when `document` is unavailable.
##########
packages/bpmn-editor/src/propertiesPanel/singleEdgeProperties/SequenceFlowProperties.tsx:
##########
@@ -39,6 +40,9 @@ export function SequenceFlowProperties({
const bpmnEditorStoreApi = useBpmnEditorStoreApi();
const isReadOnly = bpmnEditorStoreApi((s) => s.settings.isReadOnly);
+ const currentLanguage =
+ getScriptLanguage((sequenceFlow.conditionExpression as
any)?.["@_language"]) ?? ScriptLanguage.Java;
Review Comment:
`onChange` defaults `@_language` using the render-time `currentLanguage`
closure. If the user changes language and immediately types before a re-render,
this can persist the previous language back into the model. Fix by deriving the
language inside the `setState` callback from the updated element (e.g., read
`(e.conditionExpression as any)[\"@_language\"]` and normalize it), or by
passing the newly selected language through and using that value directly when
updating `@_language`.
##########
packages/bpmn-editor/src/propertiesPanel/singleEdgeProperties/SequenceFlowProperties.tsx:
##########
@@ -77,7 +96,9 @@ export function SequenceFlowProperties({
e.conditionExpression ??= {
"@_id": generateUuid(),
};
- e.conditionExpression.__$$text = newPriority;
+ e.conditionExpression.__$$text = newScript;
+ (e.conditionExpression as any)["@_xsi:type"] =
"tFormalExpression";
+ (e.conditionExpression as any)["@_language"] ??=
getScriptFormat(currentLanguage);
Review Comment:
`onChange` defaults `@_language` using the render-time `currentLanguage`
closure. If the user changes language and immediately types before a re-render,
this can persist the previous language back into the model. Fix by deriving the
language inside the `setState` callback from the updated element (e.g., read
`(e.conditionExpression as any)[\"@_language\"]` and normalize it), or by
passing the newly selected language through and using that value directly when
updating `@_language`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
