Copilot commented on code in PR #718:
URL: https://github.com/apache/shenyu-dashboard/pull/718#discussion_r4312243519
##########
src/routes/Document/components/AddAndUpdateApiDoc.js:
##########
@@ -114,6 +115,8 @@ class AddAndUpdateApiDoc extends Component {
updateExt = () => {},
} = this.props;
const { getFieldDecorator, getFieldValue } = form;
+ const extJson = parseJsonObject(ext);
+ const documentJson = parseJsonObject(document);
Review Comment:
Render-time validation now treats any non-JSON-object `ext`/`document`
(including arrays) as an error via `parseJsonObject`, but submit-time
validation (in `handleSubmit`) still uses `isJsonStr`, which accepts arrays
because `typeof [] === 'object'`. This can lead to the form displaying an error
for values like `[1,2]` while still allowing saves. Consider replacing
`isJsonStr` with `parseJsonObject(values.ext)` (and checking `.error`) so the
save-time rule matches the UI: only JSON objects should be accepted.
##########
src/routes/Document/components/AddAndUpdateApiDoc.js:
##########
@@ -386,17 +401,29 @@ class AddAndUpdateApiDoc extends Component {
<Form.Item
label={getIntlContent("SHENYU.DOCUMENT.APIDOC.DOCUMENT")}
{...formItemLayout}
+ validateStatus={documentJson.error ? "error" : undefined}
+ help={
+ documentJson.error
+ ? getIntlContent("SHENYU.PLUGIN.RULE.JSON.INVALID")
+ : undefined
+ }
Review Comment:
Render-time validation now treats any non-JSON-object `ext`/`document`
(including arrays) as an error via `parseJsonObject`, but submit-time
validation (in `handleSubmit`) still uses `isJsonStr`, which accepts arrays
because `typeof [] === 'object'`. This can lead to the form displaying an error
for values like `[1,2]` while still allowing saves. Consider replacing
`isJsonStr` with `parseJsonObject(values.ext)` (and checking `.error`) so the
save-time rule matches the UI: only JSON objects should be accepted.
##########
src/routes/Document/components/AddAndUpdateApiDoc.js:
##########
@@ -309,17 +312,29 @@ class AddAndUpdateApiDoc extends Component {
<Form.Item
label={getIntlContent("SHENYU.DOCUMENT.APIDOC.EXT")}
{...formItemLayout}
+ validateStatus={extJson.error ? "error" : undefined}
+ help={
+ extJson.error
+ ? getIntlContent("SHENYU.DOCUMENT.APIDOC.EXT.INFO")
+ : undefined
+ }
Review Comment:
Render-time validation now treats any non-JSON-object `ext`/`document`
(including arrays) as an error via `parseJsonObject`, but submit-time
validation (in `handleSubmit`) still uses `isJsonStr`, which accepts arrays
because `typeof [] === 'object'`. This can lead to the form displaying an error
for values like `[1,2]` while still allowing saves. Consider replacing
`isJsonStr` with `parseJsonObject(values.ext)` (and checking `.error`) so the
save-time rule matches the UI: only JSON objects should be accepted.
##########
src/routes/System/NamespacePlugin/AddModal.js:
##########
@@ -139,25 +152,47 @@ class AddModal extends Component {
/>,
)}
</FormItem>
- {data && data.length > 0 && (
+ {configError && (
+ <FormItem
+ label={getIntlContent("SHENYU.COMMON.SETTING")}
+ {...formItemLayout}
+ >
+ {getFieldDecorator("config", {
+ initialValue: config,
+ rules: [
+ {
+ // Only a JSON object may replace the stored config; an
+ // emptied field would erase it.
+ validator: (rule, value, callback) =>
+ callback(
+ parseJsonObject(value).value === undefined
+ ? getIntlContent("SHENYU.PLUGIN.RULE.JSON.INVALID")
+ : undefined,
+ ),
Review Comment:
Same validator concern as in `System/Plugin/AddModal.js`: prefer
`callback()` on success and `callback(message)` on failure to match Ant
Design’s documented validator contract and avoid edge-case behavior differences.
##########
src/routes/System/Plugin/AddModal.js:
##########
@@ -141,25 +153,47 @@ class AddModal extends Component {
/>,
)}
</FormItem>
- {data && data.length > 0 && (
+ {configError && (
+ <FormItem
+ label={getIntlContent("SHENYU.COMMON.SETTING")}
+ {...formItemLayout}
+ >
+ {getFieldDecorator("config", {
+ initialValue: config,
+ rules: [
+ {
+ // Only a JSON object may replace the stored config; an
+ // emptied field would erase it.
+ validator: (rule, value, callback) =>
+ callback(
+ parseJsonObject(value).value === undefined
+ ? getIntlContent("SHENYU.PLUGIN.RULE.JSON.INVALID")
+ : undefined,
+ ),
Review Comment:
Ant Design Form custom validators are expected to call `callback()` on
success and `callback(errorMessage)` on failure. Passing `undefined` as an
argument can be ambiguous across antd versions. Consider changing this to
explicitly call `callback()` on success rather than `callback(undefined)`.
##########
src/routes/System/NamespacePlugin/AddModal.test.js:
##########
@@ -73,3 +73,83 @@ it("saves actual JSON edits while preserving other stored
fields", async () => {
unknown: "keep",
});
});
+
+beforeEach(() => {
+ // validateFieldsAndScroll scrolls to the first error; jsdom has no scrollTo
+ jest.spyOn(window, "scrollTo").mockImplementation(() => {});
+});
+
Review Comment:
Same test robustness issue as `System/Plugin/AddModal.test.js`: repeatedly
`spyOn`-ing without restoring can make the suite order-dependent. Prefer an
idempotent setup with cleanup (`mockRestore`) or a simple `window.scrollTo =
jest.fn()` in a `beforeAll`.
##########
src/routes/System/Plugin/AddModal.test.js:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import React from "react";
+import { render, screen, fireEvent, waitFor } from "@testing-library/react";
+import AddModal from "./AddModal";
+
+jest.mock("dva", () => ({ connect: () => (Component) => Component }));
+jest.mock("../../../utils/IntlUtils", () => ({ getIntlContent: (key) => key
}));
+
+const fields = [
+ { field: "timeout", label: "Timeout", dataType: 2 },
+ { field: "mode", label: "Mode", dataType: 2 },
+];
+
+const baseProps = {
+ name: "divide",
+ role: "Proxy",
+ sort: 0,
+ id: "plugin-1",
+ data: fields,
+};
+
+const renderModal = (props) =>
+ render(<AddModal {...baseProps} handleOk={jest.fn()} {...props} />);
+
+const save = () =>
+ fireEvent.click(screen.getByRole("button", { name: "SHENYU.COMMON.SURE" }));
+
+it("renders schema fields from a stored JSON object", async () => {
+ const handleOk = jest.fn();
+ renderModal({ config: '{"timeout":5,"unknown":"keep"}', handleOk });
+ expect(screen.getByPlaceholderText("Timeout").value).toBe("5");
+ expect(
+ screen.queryByDisplayValue('{"timeout":5,"unknown":"keep"}'),
+ ).toBeNull();
+ save();
+ await waitFor(() => expect(handleOk).toHaveBeenCalledTimes(1));
+ expect(JSON.parse(handleOk.mock.calls[0][0].config)).toEqual({
+ timeout: 5,
+ unknown: "keep",
+ });
+});
+
+it.each([undefined, null, ""])(
+ "treats a %p config as empty and still saves",
+ async (config) => {
+ const handleOk = jest.fn();
+ renderModal({ config, handleOk });
+ expect(screen.getByPlaceholderText("Timeout").value).toBe("");
+ expect(
+ screen.queryByRole("textbox", { name: "SHENYU.COMMON.SETTING" }),
+ ).toBeNull();
+ save();
+ await waitFor(() => expect(handleOk).toHaveBeenCalledTimes(1));
+ },
+);
+
+beforeEach(() => {
+ // validateFieldsAndScroll scrolls to the first error; jsdom has no scrollTo
+ jest.spyOn(window, "scrollTo").mockImplementation(() => {});
+});
+
Review Comment:
This `beforeEach` creates a new spy repeatedly and is not restored, which
can lead to brittle tests (or failures if the method is already mocked/spyed by
another suite). Consider using `beforeAll` + `afterAll`/`afterEach` to restore
the spy, or assigning `window.scrollTo = jest.fn()` (guarded if not present) to
make the setup idempotent.
--
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]
