Copilot commented on code in PR #3292: URL: https://github.com/apache/apisix-dashboard/pull/3292#discussion_r2802081734
########## src/utils/route-transformer.ts: ########## @@ -0,0 +1,264 @@ +/** + * 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. + */ + +/** + * Route Transformer Module + * + * Provides symmetric (reversible) transformations between: + * - API format: What the server sends/receives + * - Internal format: Canonical representation (single source of truth) + * - Form format: What React Hook Form uses + * - JSON View format: What user sees in JSON editor + * + * Key principle: vars is ALWAYS an array in internal/API/JSON formats, + * but a JSON string in form format (for Monaco editor compatibility) + */ + +import { produce } from 'immer'; +import { isNotEmpty } from 'rambdax'; + +import type { RoutePutType } from '@/components/form-slice/FormPartRoute/schema'; +import type { APISIXType } from '@/types/schema/apisix'; + +import { deepCleanEmptyKeys, rmDoubleUnderscoreKeys } from './producer'; + +/** + * Internal canonical format for Route + * vars is always an array, never a string + */ +export type RouteInternal = Omit<APISIXType['Route'], 'vars'> & { + vars?: unknown[]; + // Form-specific fields (prefixed with __) + __checksEnabled?: boolean; + __checksPassiveEnabled?: boolean; +}; + +/** + * Transform API data to internal format + * This is essentially an identity transformation since API already has vars as array + */ +export function apiToInternal(api: APISIXType['Route']): RouteInternal { + return { + ...api, + vars: api.vars as unknown[] | undefined, + }; +} + +/** + * Transform internal format to API format + * Removes form-specific fields (prefixed with __) and cleans empty keys + */ +export function internalToApi(internal: RouteInternal): APISIXType['Route'] { + return produce(internal, (draft) => { + // Remove __ prefixed fields + rmDoubleUnderscoreKeys(draft); + + // Remove timestamps (server manages these) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const mutableDraft = draft as any; + delete mutableDraft.create_time; + delete mutableDraft.update_time; + + // Remove upstream if service_id or upstream_id is set + if ((draft.service_id || draft.upstream_id) && isNotEmpty(draft.upstream)) { + delete draft.upstream; + } + + // Clean empty keys + deepCleanEmptyKeys(draft); + }) as APISIXType['Route']; +} + +/** + * Transform internal format to form format + * Key change: vars array is stringified for Monaco editor + */ +export function internalToForm(internal: RouteInternal): RoutePutType { + // Deep clone to avoid mutation issues with frozen objects + const cloned = JSON.parse(JSON.stringify(internal)) as RoutePutType; + + // Stringify vars array for form editor + if (cloned.vars && Array.isArray(cloned.vars)) { + cloned.vars = JSON.stringify(cloned.vars); + } + + // Add form-specific upstream checks flags + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const mutableCloned = cloned as any; + const upstream = internal.upstream; + if (upstream) { + mutableCloned.__checksEnabled = !!upstream.checks && isNotEmpty(upstream.checks); + mutableCloned.__checksPassiveEnabled = + !!upstream.checks?.passive && isNotEmpty(upstream.checks.passive); + } + + return cloned; +} + +/** + * Transform form format to internal format + * Key change: vars string is parsed back to array + */ +export function formToInternal(form: RoutePutType): RouteInternal { + // Deep clone to avoid mutation issues with frozen objects + const cloned = JSON.parse(JSON.stringify(form)) as RouteInternal; + + // Parse vars string back to array + if (cloned.vars && typeof cloned.vars === 'string') { + try { + cloned.vars = JSON.parse(cloned.vars as string); + } catch { + // Keep as-is if parse fails (will be caught by validation) + cloned.vars = []; + } Review Comment: On JSON parse failure for `vars`, this silently sets `vars` to an empty array. Because the form schema defines `vars` as an optional string (no JSON validation), invalid user input will be dropped and an empty `vars` will be saved without any error. Prefer surfacing the parse error (e.g., keep the original string and let validation fail, or throw/return an error so the UI can block save and show feedback). ########## src/hooks/useDirtyState.ts: ########## @@ -0,0 +1,220 @@ +/** + * 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 { useCallback, useMemo, useRef, useState } from 'react'; + +/** + * Deep equality comparison for objects and arrays + */ +function deepEqual(a: unknown, b: unknown): boolean { + if (a === b) return true; + + if (typeof a !== typeof b) return false; + + if (a === null || b === null) return a === b; + + if (typeof a !== 'object') return false; + + if (Array.isArray(a) !== Array.isArray(b)) return false; + + if (Array.isArray(a) && Array.isArray(b)) { + if (a.length !== b.length) return false; + return a.every((item, index) => deepEqual(item, b[index])); + } + + const keysA = Object.keys(a as object); + const keysB = Object.keys(b as object); + + if (keysA.length !== keysB.length) return false; + + return keysA.every((key) => + deepEqual( + (a as Record<string, unknown>)[key], + (b as Record<string, unknown>)[key] + ) + ); +} + +/** + * Get changed fields between two objects + */ +function getChangedFields( + original: unknown, + current: unknown, + prefix = '' +): string[] { + if (typeof original !== 'object' || original === null) { + return deepEqual(original, current) ? [] : [prefix || 'value']; + } + + if (typeof current !== 'object' || current === null) { + return [prefix || 'value']; + } + + const changedFields: string[] = []; + const allKeys = new Set([ + ...Object.keys(original as object), + ...Object.keys(current as object), + ]); + + allKeys.forEach((key) => { + const origVal = (original as Record<string, unknown>)[key]; + const currVal = (current as Record<string, unknown>)[key]; + const fieldPath = prefix ? `${prefix}.${key}` : key; + + if (!deepEqual(origVal, currVal)) { + if ( + typeof origVal === 'object' && + origVal !== null && + typeof currVal === 'object' && + currVal !== null + ) { + changedFields.push(...getChangedFields(origVal, currVal, fieldPath)); + } else { + changedFields.push(fieldPath); + } + } + }); + + return changedFields; +} + +export interface DirtyState<T> { + /** Original value (from server) */ + original: T | undefined; + /** Current value (with local edits) */ + current: T | undefined; + /** Whether current differs from original */ + isDirty: boolean; + /** List of field paths that have changed */ + changedFields: string[]; + /** Update current value */ + update: (newValue: T) => void; + /** Reset to a new original value (and clear dirty state) */ + reset: (newOriginal?: T) => void; + /** Check if a specific field is dirty */ + isFieldDirty: (fieldPath: string) => boolean; +} + +/** + * Hook for tracking dirty state with deep comparison + * + * @param initialOriginal - The original value to compare against + * @returns DirtyState object with current value, isDirty flag, and update functions + * + * @example + * ```tsx + * const { current, isDirty, update, reset } = useDirtyState(serverData); + * + * // Update current value + * update({ ...current, name: 'New Name' }); + * + * // Check if dirty + * if (isDirty) { + * // Show warning before navigation + * } + * + * // Reset after save + * reset(savedData); + * ``` + */ +export function useDirtyState<T>(initialOriginal: T | undefined): DirtyState<T> { + const [original, setOriginal] = useState<T | undefined>(initialOriginal); + const [current, setCurrent] = useState<T | undefined>(initialOriginal); + + // Use ref to track if we've initialized with initialOriginal + const initializedRef = useRef(false); + + // Update original when initialOriginal changes (e.g., after data fetch) + if (initialOriginal !== undefined && !initializedRef.current) { + initializedRef.current = true; + setOriginal(initialOriginal); + setCurrent(initialOriginal); + } Review Comment: `useDirtyState` is calling `setOriginal`/`setCurrent` during render when `initialOriginal` arrives. This can trigger React warnings (“Cannot update a component while rendering…”) and can lead to render loops. Move this initialization into a `useEffect` that runs when `initialOriginal` changes (and reset `initializedRef` appropriately if you need to handle refetches). ########## src/components/page/TableActionMenu.tsx: ########## @@ -0,0 +1,104 @@ +/** + * 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 { ActionIcon, Menu, Text } from '@mantine/core'; +import { modals } from '@mantine/modals'; +import { notifications } from '@mantine/notifications'; +import { useTranslation } from 'react-i18next'; + +import { queryClient } from '@/config/global'; +import { req } from '@/config/req'; +import IconCode from '~icons/material-symbols/code'; +import IconDelete from '~icons/material-symbols/delete'; +import IconForm from '~icons/material-symbols/list-alt'; +import IconMore from '~icons/material-symbols/more-vert'; + +export type TableActionMenuProps = { + /** Resource name for delete confirmation (e.g., "Route") */ + resourceName: string; + /** Resource ID or name for delete confirmation display */ + resourceTarget: string; + /** API endpoint for delete request */ + deleteApi: string; + /** Callback after successful deletion */ + onDeleteSuccess: () => void; + /** Callback when Form edit is clicked (opens drawer) */ + onFormEdit: () => void; + /** Callback when JSON edit is clicked (opens drawer) */ + onJsonEdit: () => void; +}; + +export const TableActionMenu = (props: TableActionMenuProps) => { + const { resourceName, resourceTarget, deleteApi, onDeleteSuccess, onFormEdit, onJsonEdit } = props; + const { t } = useTranslation(); + + const handleDeleteClick = () => { + modals.openConfirmModal({ + centered: true, + confirmProps: { color: 'red' }, + title: t('info.delete.title', { name: resourceName }), + children: ( + <Text> + {t('info.delete.content', { name: resourceName })} + <Text + component="span" + fw={700} + mx="0.25em" + style={{ wordBreak: 'break-all' }} + > + {resourceTarget} + </Text> + {t('mark.question')} + </Text> + ), + labels: { confirm: t('form.btn.delete'), cancel: t('form.btn.cancel') }, + onConfirm: () => + req + .delete(deleteApi) + .then(() => onDeleteSuccess?.()) + .then(() => { + notifications.show({ + message: t('info.delete.success', { name: resourceName }), + color: 'green', + }); + queryClient.invalidateQueries(); + }), Review Comment: `queryClient.invalidateQueries()` without a `queryKey` will invalidate *all* queries after every delete, which can cause unnecessary refetching and UI churn. Consider scoping invalidation to the relevant resource/list query keys (or accept a `queryKey`/`invalidateKeys` prop) and rely on `onDeleteSuccess` for the local list refresh. ########## src/components/page/ToAddPageBtn.tsx: ########## @@ -40,6 +45,49 @@ export const ToAddPageBtn = ({ to, params, label }: ToAddPageBtnProps) => { ); }; +export type ToAddPageDropdownProps = { + to: string; + label: string; + params?: Record<string, string>; +}; + +export const ToAddPageDropdown = ({ to, label, params }: ToAddPageDropdownProps) => { + const { t } = useTranslation(); + const navigate = useNavigate(); + + const handleFormClick = () => { + navigate({ to, params }); + }; + + const handleJsonClick = () => { + navigate({ to, params, search: { mode: 'json' } }); + }; + + return ( + <Menu shadow="md" width={200}> + <Menu.Target> + <Button + leftSection={<IconPlus />} + rightSection={<IconChevronDown />} + size="compact-sm" + variant="gradient" + > + {label} + </Button> + </Menu.Target> Review Comment: The new add button requires two clicks to reach the default (form) flow because clicking the button only opens the menu. This is a behavior change from the previous single-click navigation and is likely to break existing e2e flows and user muscle memory. Consider a split-button approach (primary click navigates to form; chevron opens the menu) or make the main button click default to form and keep JSON as the secondary menu option. ########## src/utils/json-transformer.ts: ########## @@ -0,0 +1,99 @@ +/** + * 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. + */ + +/** + * JSON Transformer Utilities + * + * Provides bidirectional transformation between form state and JSON representation + * for APISIX Dashboard resources. + */ + +/** + * Convert form data to formatted JSON string + * + * @param formData - The form data object + * @param produceFn - Optional producer function to transform data before serialization + * @returns Formatted JSON string with 2-space indentation + */ +export const formToJSON = <T>( + formData: T, + produceFn?: (data: T) => unknown +): string => { + try { + // Apply producer function if provided (e.g., produceRoute) + const transformedData = produceFn ? produceFn(formData) : formData; + + // Convert to JSON with 2-space indentation for readability + return JSON.stringify(transformedData, null, 2); + } catch { + throw new Error('Failed to convert form data to JSON'); + } +}; + +/** + * Parse and validate JSON string, converting it to form data format + * + * @param jsonString - The JSON string to parse + * @param parseFormFn - Optional function to transform parsed data to form format + * @returns Parsed and transformed form data + * @throws Error if JSON is invalid or transformation fails + */ +export const jsonToForm = <T>( + jsonString: string, + parseFormFn?: (data: unknown) => T +): T => { + try { + // Parse JSON string + const parsed = JSON.parse(jsonString); + + // Apply form transformation if provided (e.g., produceVarsToForm) + const formData = parseFormFn ? parseFormFn(parsed) : parsed; + + return formData as T; + } catch (error) { + if (error instanceof SyntaxError) { + throw new Error('Invalid JSON syntax'); + } + throw new Error('Failed to convert JSON to form data'); + } +}; + +/** + * Validate JSON syntax without parsing + * + * @param jsonString - The JSON string to validate + * @returns Object with isValid flag and optional error message + */ +export const validateJSONSyntax = ( + jsonString: string +): { isValid: boolean; error?: string } => { + try { + JSON.parse(jsonString); + return { isValid: true }; Review Comment: The comment says “Validate JSON syntax without parsing”, but the implementation uses `JSON.parse`, which does parse the JSON. Either adjust the comment (e.g., “validate by attempting to parse”) or switch to a true syntax-only validator if that’s the intent. ########## src/components/page/ToAddPageBtn.tsx: ########## @@ -55,3 +103,44 @@ export const ToDetailPageBtn = (props: ToDetailPageBtnProps) => { </RouteLinkBtn> ); }; + +export type ToDetailPageDropdownProps = { + to: string; + params: Record<string, string>; +}; + +export const ToDetailPageDropdown = ({ to, params }: ToDetailPageDropdownProps) => { + const { t } = useTranslation(); + const navigate = useNavigate(); + + const handleFormClick = () => { + navigate({ to, params }); + }; + + const handleJsonClick = () => { + navigate({ to, params, search: { mode: 'json' } }); + }; + + return ( + <Menu shadow="md" width={200}> + <Menu.Target> + <Button + rightSection={<IconChevronDown />} + size="compact-xs" + variant="light" + > + {t('form.btn.edit')} + </Button> + </Menu.Target> + + <Menu.Dropdown> + <Menu.Item leftSection={<IconForm />} onClick={handleFormClick}> + {t('form.view.editWithForm')} + </Menu.Item> + <Menu.Item leftSection={<IconCode />} onClick={handleJsonClick}> + {t('form.view.editWithJSON')} Review Comment: `ToAddPageDropdown`/`ToDetailPageDropdown` use i18n keys like `form.view.createWithForm`, `createWithJSON`, `editWithForm`, and `editWithJSON`. These keys were added in `en/common.json` but are currently missing from other locale files updated in this PR (e.g., `de`, `es`, `tr`, `zh`), which will cause raw key strings to show in those languages. Add the missing translations (or provide a fallback strategy). ```suggestion {t('form.view.editWithForm', 'Edit with form')} </Menu.Item> <Menu.Item leftSection={<IconCode />} onClick={handleJsonClick}> {t('form.view.editWithJSON', 'Edit with JSON')} ``` -- 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]
