Copilot commented on code in PR #711: URL: https://github.com/apache/shenyu-dashboard/pull/711#discussion_r4229365547
########## src/utils/loading.js: ########## @@ -0,0 +1,48 @@ +/* + * 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 createDvaLoading from "dva-loading"; + +// Action types used by dva-loading's reducer (not exported by the package). +const SHOW = "@@DVA_LOADING/SHOW"; +const HIDE = "@@DVA_LOADING/HIDE"; + +// dva-loading 2.x dispatches HIDE only after the effect returns, so an effect +// that throws (e.g. a rejected request) or is cancelled leaves its loading +// flags set forever. Keep its reducer and only/except filtering, but always +// dispatch HIDE from a finally block. +export default function createLoading(opts = {}) { + const loading = createDvaLoading(opts); + + function onEffect(effect, sagaEffects, model, actionType) { + if (loading.onEffect(effect, sagaEffects, model, actionType) === effect) { + return effect; + } + const { put } = sagaEffects; + const payload = { namespace: model.namespace, actionType }; + return function* loadingEffect(...args) { + yield put({ type: SHOW, payload }); + try { + yield effect(...args); + } finally { + yield put({ type: HIDE, payload }); + } + }; Review Comment: The wrapper currently discards the wrapped effect’s return value because it does not return the result of `effect(...args)`. In dva, `dispatch()` typically resolves to the effect’s return value, so this can subtly change behavior for callers awaiting a result. Preserve the return value by returning the yielded result from the wrapped effect (e.g., capture the result in a variable and return it, or `return yield effect(...args)` inside the `try`). ########## src/utils/loading.test.js: ########## @@ -0,0 +1,110 @@ +/* + * 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 { runSaga, effects } from "dva/saga"; +import createLoading from "./loading"; +import globalModel from "../models/global"; +import { queryPlatform } from "../services/api"; + +jest.mock("../services/api", () => ({ queryPlatform: jest.fn() })); +jest.mock("antd", () => ({ message: { warn: jest.fn(), success: jest.fn() } })); +jest.mock("../utils/IntlUtils", () => ({ getIntlContent: (key) => key })); +jest.mock("../components/_utils/utils", () => ({ + defaultNamespaceId: "default-namespace", +})); + +function setup(opts) { + const loading = createLoading(opts); + const reducer = loading.extraReducers.loading; + let state = reducer(undefined, { type: "@@INIT" }); + const actions = []; + const dispatch = (action) => { + actions.push(action.type); + state = reducer(state, action); + }; + const run = (effect, model, actionType) => + runSaga( + { dispatch, getState: () => ({}), onError: () => {} }, + loading.onEffect(effect, effects, model, actionType), + { type: actionType }, + effects, + ); + return { loading, run, actions, getState: () => state }; +} + +describe("createLoading", () => { + it("hides loading after an effect resolves", async () => { + const { run, actions, getState } = setup(); + function* ok() { + yield effects.call(() => Promise.resolve()); + } + + await run(ok, { namespace: "demo" }, "demo/ok").done; + + expect(actions).toEqual(["@@DVA_LOADING/SHOW", "@@DVA_LOADING/HIDE"]); + expect(getState()).toEqual({ + global: false, + models: { demo: false }, + effects: { "demo/ok": false }, + }); + }); + + it("hides loading when a request rejects inside an effect", async () => { + queryPlatform.mockRejectedValue(new Error("network down")); + const { run, actions, getState } = setup(); + + await expect( + run( + globalModel.effects.fetchPlatform, + globalModel, + "global/fetchPlatform", + ).done, + ).rejects.toThrow("network down"); + + expect(actions).toEqual(["@@DVA_LOADING/SHOW", "@@DVA_LOADING/HIDE"]); + expect(getState()).toEqual({ + global: false, + models: { global: false }, + effects: { "global/fetchPlatform": false }, + }); + }); + + it("hides loading when an effect is cancelled", () => { + const { run, getState } = setup(); + function* pending() { + yield effects.call(() => new Promise(() => {})); + } + + const task = run(pending, { namespace: "demo" }, "demo/pending"); + expect(getState().effects["demo/pending"]).toBe(true); + task.cancel(); + + expect(getState().global).toBe(false); + expect(getState().effects["demo/pending"]).toBe(false); + }); Review Comment: This cancellation test can be flaky because `task.cancel()` does not guarantee the `finally` block (and the resulting `put(HIDE)`) has been processed synchronously before the assertions run. Make the test `async` and await the task settling (e.g., await the task promise and ignore the cancellation rejection, or await a microtask tick) before asserting on state, so the test reliably observes the `HIDE` dispatch. -- 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]
