From 4e3e24bc81dd219c0f06d342c56495bb23c6f163 Mon Sep 17 00:00:00 2001 From: cxymds Date: Tue, 21 Jul 2026 23:35:33 +0800 Subject: [PATCH] fix(events): fail closed on unknown notify state --- app/(dashboard)/events-target/page.tsx | 104 ++++++++++----- hooks/use-event-target.ts | 1 + lib/event-destinations-access.js | 3 - lib/event-destinations-access.ts | 3 - lib/event-destinations-state.ts | 80 ++++++++++++ lib/notify-module-access.js | 2 +- lib/notify-module-access.ts | 2 +- tests/lib/event-destinations-access.test.js | 10 -- tests/lib/event-destinations-state.test.ts | 134 ++++++++++++++++++++ tests/lib/notify-module-access.test.js | 4 +- 10 files changed, 294 insertions(+), 49 deletions(-) delete mode 100644 lib/event-destinations-access.js delete mode 100644 lib/event-destinations-access.ts create mode 100644 lib/event-destinations-state.ts delete mode 100644 tests/lib/event-destinations-access.test.js create mode 100644 tests/lib/event-destinations-state.test.ts diff --git a/app/(dashboard)/events-target/page.tsx b/app/(dashboard)/events-target/page.tsx index 767be379..79e5ac0c 100644 --- a/app/(dashboard)/events-target/page.tsx +++ b/app/(dashboard)/events-target/page.tsx @@ -15,7 +15,13 @@ import { useDataTable } from "@/hooks/use-data-table" import { useEventTarget } from "@/hooks/use-event-target" import { useModuleSwitches } from "@/hooks/use-module-switches" import { EventsTargetNewForm } from "@/components/events-target/new-form" -import { canManageEventDestinations } from "@/lib/event-destinations-access" +import { + applyEventDestinationsLoadResult, + canManageEventDestinations, + initialEventDestinationsState, + resolveEventDestinationsNotifyState, + type EventDestinationsLoadState, +} from "@/lib/event-destinations-state" import { useDialog } from "@/lib/feedback/dialog" import { useMessage } from "@/lib/feedback/message" import type { ColumnDef } from "@tanstack/react-table" @@ -38,41 +44,60 @@ export default function EventsTargetPage() { const { getEventsTargetList, deleteEventTarget } = useEventTarget() const { getModuleSwitches } = useModuleSwitches() - const [data, setData] = useState([]) - const [loading, setLoading] = useState(false) + const [loadState, setLoadState] = useState>({ + ...initialEventDestinationsState, + }) + const [loading, setLoading] = useState(true) const [searchTerm, setSearchTerm] = useState("") const [newFormOpen, setNewFormOpen] = useState(false) - const [notifyEnabled, setNotifyEnabled] = useState(undefined) + const loadingRef = React.useRef(false) + const requestVersionRef = React.useRef(0) - const canManageDestinations = canManageEventDestinations(notifyEnabled) + const { data, loadFailed, notifyEnabled, notifyFailed } = loadState + const canManageDestinations = canManageEventDestinations({ + notifyEnabled, + loading, + loadFailed, + notifyFailed, + }) const loadData = useCallback(async () => { + if (loadingRef.current) return + loadingRef.current = true + const requestVersion = ++requestVersionRef.current setLoading(true) try { - const [response, switches] = await Promise.allSettled([ - getEventsTargetList(), + const response = await getEventsTargetList() + const nextNotifyEnabled = await resolveEventDestinationsNotifyState(response, () => getModuleSwitches({ suppress403Redirect: true }), - ]) - - if (switches.status === "fulfilled" && switches.value) { - setNotifyEnabled(switches.value.notify_enabled) - } + ) - if (response.status === "rejected") { - throw response.reason - } - - const list = (response.value?.notification_endpoints ?? []) as RowData[] - setData(list) + if (requestVersion !== requestVersionRef.current) return + const list = (response.notification_endpoints ?? []) as RowData[] + setLoadState((current) => + applyEventDestinationsLoadResult(current, { + ok: true, + data: list, + notifyEnabled: nextNotifyEnabled, + }), + ) } catch { - setData([]) + if (requestVersion !== requestVersionRef.current) return + setLoadState((current) => applyEventDestinationsLoadResult(current, { ok: false })) } finally { - setLoading(false) + if (requestVersion === requestVersionRef.current) { + loadingRef.current = false + setLoading(false) + } } }, [getEventsTargetList, getModuleSwitches]) useEffect(() => { loadData() + return () => { + requestVersionRef.current += 1 + loadingRef.current = false + } }, [loadData]) const filteredData = useMemo(() => { @@ -209,9 +234,9 @@ export default function EventsTargetPage() { {t("Add Event Destination")} - @@ -220,19 +245,40 @@ export default function EventsTargetPage() {

{t("Event Destinations")}

- {!canManageDestinations ? ( + {notifyEnabled === false ? ( {t("Notify is disabled")} {t("Enable notify in Settings before managing event destinations.")} ) : null} - + {notifyFailed ? ( + + {t("Notify status unavailable")} + + {t("Unable to verify whether notify is enabled. Refresh before making changes.")} + + + ) : null} + + {loadFailed ? ( + + {t("Load Failed")} + + {data.length > 0 ? t("Previously loaded values may be stale.") : t("Refresh to try again.")} + + + ) : null} + + {data.length > 0 || !loadFailed ? ( + + ) : null} diff --git a/hooks/use-event-target.ts b/hooks/use-event-target.ts index 76253116..e15ac2ce 100644 --- a/hooks/use-event-target.ts +++ b/hooks/use-event-target.ts @@ -8,6 +8,7 @@ export function useEventTarget() { const getEventsTargetList = useCallback(async () => { return api.get("/target/list") as Promise<{ + notify_enabled?: unknown notification_endpoints?: Array<{ account_id: string service: string diff --git a/lib/event-destinations-access.js b/lib/event-destinations-access.js deleted file mode 100644 index cee26145..00000000 --- a/lib/event-destinations-access.js +++ /dev/null @@ -1,3 +0,0 @@ -import { canManageNotifyBackedFeature } from "./notify-module-access.js" - -export const canManageEventDestinations = canManageNotifyBackedFeature diff --git a/lib/event-destinations-access.ts b/lib/event-destinations-access.ts deleted file mode 100644 index 1add8af4..00000000 --- a/lib/event-destinations-access.ts +++ /dev/null @@ -1,3 +0,0 @@ -import { canManageNotifyBackedFeature } from "@/lib/notify-module-access" - -export const canManageEventDestinations = canManageNotifyBackedFeature diff --git a/lib/event-destinations-state.ts b/lib/event-destinations-state.ts new file mode 100644 index 00000000..4dda9bb3 --- /dev/null +++ b/lib/event-destinations-state.ts @@ -0,0 +1,80 @@ +export interface EventDestinationsLoadState { + data: T[] + loadFailed: boolean + notifyEnabled: boolean | undefined + notifyFailed: boolean +} + +export const initialEventDestinationsState: EventDestinationsLoadState = { + data: [], + loadFailed: false, + notifyEnabled: undefined, + notifyFailed: false, +} + +export function readTargetListNotifyState(payload: unknown): { + present: boolean + value: boolean | undefined +} { + if (!payload || typeof payload !== "object" || !Object.hasOwn(payload, "notify_enabled")) { + return { present: false, value: undefined } + } + + const value = (payload as { notify_enabled?: unknown }).notify_enabled + return { present: true, value: typeof value === "boolean" ? value : undefined } +} + +export function readModuleNotifyState(payload: unknown) { + if (!payload || typeof payload !== "object") return undefined + const value = (payload as { notify_enabled?: unknown }).notify_enabled + return typeof value === "boolean" ? value : undefined +} + +export async function resolveEventDestinationsNotifyState( + targetListPayload: unknown, + loadModuleSwitches: () => Promise, +) { + const targetListState = readTargetListNotifyState(targetListPayload) + if (targetListState.present) return targetListState.value + + try { + return readModuleNotifyState(await loadModuleSwitches()) + } catch { + return undefined + } +} + +export function applyEventDestinationsLoadResult( + current: EventDestinationsLoadState, + result: { ok: true; data: T[]; notifyEnabled: boolean | undefined } | { ok: false }, +): EventDestinationsLoadState { + if (!result.ok) { + return { + ...current, + loadFailed: true, + notifyEnabled: undefined, + notifyFailed: false, + } + } + + return { + data: result.data, + loadFailed: false, + notifyEnabled: result.notifyEnabled, + notifyFailed: result.notifyEnabled === undefined, + } +} + +export function canManageEventDestinations({ + notifyEnabled, + loading, + loadFailed, + notifyFailed, +}: { + notifyEnabled: boolean | undefined + loading: boolean + loadFailed: boolean + notifyFailed: boolean +}) { + return notifyEnabled === true && !loading && !loadFailed && !notifyFailed +} diff --git a/lib/notify-module-access.js b/lib/notify-module-access.js index 437d6ce7..48d86916 100644 --- a/lib/notify-module-access.js +++ b/lib/notify-module-access.js @@ -1,3 +1,3 @@ export function canManageNotifyBackedFeature(notifyEnabled) { - return notifyEnabled !== false + return notifyEnabled === true } diff --git a/lib/notify-module-access.ts b/lib/notify-module-access.ts index cd50ba12..77c2e18a 100644 --- a/lib/notify-module-access.ts +++ b/lib/notify-module-access.ts @@ -1,3 +1,3 @@ export function canManageNotifyBackedFeature(notifyEnabled: boolean | undefined) { - return notifyEnabled !== false + return notifyEnabled === true } diff --git a/tests/lib/event-destinations-access.test.js b/tests/lib/event-destinations-access.test.js deleted file mode 100644 index 780d0498..00000000 --- a/tests/lib/event-destinations-access.test.js +++ /dev/null @@ -1,10 +0,0 @@ -import test from "node:test" -import assert from "node:assert/strict" - -import { canManageEventDestinations } from "../../lib/event-destinations-access.js" - -test("canManageEventDestinations blocks changes only when notify is disabled", () => { - assert.equal(canManageEventDestinations(false), false) - assert.equal(canManageEventDestinations(true), true) - assert.equal(canManageEventDestinations(undefined), true) -}) diff --git a/tests/lib/event-destinations-state.test.ts b/tests/lib/event-destinations-state.test.ts new file mode 100644 index 00000000..59eb7278 --- /dev/null +++ b/tests/lib/event-destinations-state.test.ts @@ -0,0 +1,134 @@ +import assert from "node:assert/strict" +import { readFile } from "node:fs/promises" +import test from "node:test" + +const loadStateModule = () => import(new URL("../../lib/event-destinations-state.ts", import.meta.url).href) + +test("target-list notify state accepts only literal booleans", async () => { + const { readTargetListNotifyState } = await loadStateModule() + + for (const [value, expected] of [ + [true, true], + [false, false], + [undefined, undefined], + [null, undefined], + ["true", undefined], + [1, undefined], + ] as const) { + assert.deepEqual(readTargetListNotifyState({ notify_enabled: value }), { + present: true, + value: expected, + }) + } +}) + +test("target-list notify state falls back only when the field is absent", async () => { + const { readTargetListNotifyState, resolveEventDestinationsNotifyState } = await loadStateModule() + let fallbackCalls = 0 + + assert.deepEqual(readTargetListNotifyState({ notification_endpoints: [] }), { + present: false, + value: undefined, + }) + assert.equal( + await resolveEventDestinationsNotifyState({ notification_endpoints: [] }, async () => { + fallbackCalls += 1 + return { notify_enabled: true } + }), + true, + ) + assert.equal( + await resolveEventDestinationsNotifyState({ notify_enabled: null }, async () => { + fallbackCalls += 1 + return { notify_enabled: true } + }), + undefined, + ) + assert.equal(fallbackCalls, 1) +}) + +test("module-switch authorization and network failures remain unknown", async () => { + const { resolveEventDestinationsNotifyState } = await loadStateModule() + + for (const failure of [new Error("403"), new Error("network unavailable")]) { + assert.equal( + await resolveEventDestinationsNotifyState({}, async () => { + throw failure + }), + undefined, + ) + } + assert.equal(await resolveEventDestinationsNotifyState({}, async () => ({ notify_enabled: "true" })), undefined) +}) + +test("destination writes require trusted notify and list state", async () => { + const { canManageEventDestinations } = await loadStateModule() + const trusted = { + notifyEnabled: true, + loading: false, + loadFailed: false, + notifyFailed: false, + } + + assert.equal(canManageEventDestinations(trusted), true) + assert.equal(canManageEventDestinations({ ...trusted, notifyEnabled: false }), false) + assert.equal(canManageEventDestinations({ ...trusted, notifyEnabled: undefined }), false) + assert.equal(canManageEventDestinations({ ...trusted, loading: true }), false) + assert.equal(canManageEventDestinations({ ...trusted, loadFailed: true }), false) + assert.equal(canManageEventDestinations({ ...trusted, notifyFailed: true }), false) +}) + +test("list failure preserves trusted data and successful retry recovers", async () => { + const { applyEventDestinationsLoadResult, initialEventDestinationsState } = await loadStateModule() + const loaded = applyEventDestinationsLoadResult(initialEventDestinationsState, { + ok: true, + data: [{ account_id: "primary" }], + notifyEnabled: true, + }) + const failed = applyEventDestinationsLoadResult(loaded, { ok: false }) + const recovered = applyEventDestinationsLoadResult(failed, { + ok: true, + data: [{ account_id: "recovered" }], + notifyEnabled: true, + }) + + assert.deepEqual(failed, { + data: [{ account_id: "primary" }], + loadFailed: true, + notifyEnabled: undefined, + notifyFailed: false, + }) + assert.deepEqual(recovered, { + data: [{ account_id: "recovered" }], + loadFailed: false, + notifyEnabled: true, + notifyFailed: false, + }) +}) + +test("invalid or unavailable notify state remains unknown after a successful list", async () => { + const { applyEventDestinationsLoadResult, initialEventDestinationsState } = await loadStateModule() + + assert.deepEqual( + applyEventDestinationsLoadResult(initialEventDestinationsState, { + ok: true, + data: [{ account_id: "primary" }], + notifyEnabled: undefined, + }), + { + data: [{ account_id: "primary" }], + loadFailed: false, + notifyEnabled: undefined, + notifyFailed: true, + }, + ) +}) + +test("destination page blocks duplicate refresh and does not render a failed first load as empty", async () => { + const source = await readFile(new URL("../../app/(dashboard)/events-target/page.tsx", import.meta.url), "utf8") + + assert.match(source, /if \(loadingRef\.current\) return/) + assert.match(source, /onClick=\{loadData\} disabled=\{loading\}/) + assert.match(source, /data\.length > 0 \|\| !loadFailed/) + assert.match(source, /useState\(true\)/) +}) diff --git a/tests/lib/notify-module-access.test.js b/tests/lib/notify-module-access.test.js index 1b1ca2df..6faf8f5a 100644 --- a/tests/lib/notify-module-access.test.js +++ b/tests/lib/notify-module-access.test.js @@ -3,8 +3,8 @@ import assert from "node:assert/strict" import { canManageNotifyBackedFeature } from "../../lib/notify-module-access.js" -test("canManageNotifyBackedFeature blocks changes only when notify is disabled", () => { +test("canManageNotifyBackedFeature requires notify to be explicitly enabled", () => { assert.equal(canManageNotifyBackedFeature(false), false) assert.equal(canManageNotifyBackedFeature(true), true) - assert.equal(canManageNotifyBackedFeature(undefined), true) + assert.equal(canManageNotifyBackedFeature(undefined), false) })