laserninja commented on code in PR #12900:
URL: https://github.com/apache/gravitino/pull/12900#discussion_r4253954678


##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -2332,6 +2374,136 @@ export const deleteView = createAsyncThunk(
   }
 )
 
+export const fetchSemanticModels = createAsyncThunk(
+  'appMetalakes/fetchSemanticModels',
+  async ({ init, metalake, catalog, schema }, { getState, dispatch }) => {
+    const [err, res] = await to(getSemanticModelsApi({ metalake, catalog, 
schema }, { errorMessageMode: 'none' }))
+
+    if (err || !res) {
+      // The catalog doesn't support semantic models (405), or the server 
predates the endpoint and
+      // has no route for it (404). Both mean "this schema has none" — return 
empty silently rather
+      // than failing every schema page against an older server.
+      if ([404, 405].includes(err?.response?.status)) {
+        return { semanticModels: [], init }
+      }
+      if (init) {
+        throw new Error(err)
+      }
+    }
+
+    const { identifiers = [] } = res || {}
+
+    const semanticModels = identifiers.map(semanticModelItem => {
+      return {
+        ...semanticModelItem,
+        node: 'semanticModel',
+        id: 
`{{${metalake}}}{{${catalog}}}{{${'relational'}}}{{${schema}}}{{${semanticModelItem.name}}}`,
+        key: 
`{{${metalake}}}{{${catalog}}}{{${'relational'}}}{{${schema}}}{{${semanticModelItem.name}}}`,

Review Comment:
   Fixed in 6f82c2c86. Both node builders and both route-to-selection mappings 
now use a shared key helper that includes the Semantic Model entity type. Added 
a regression test for a same-named table and model across schema expansion and 
list refreshes. Also verified in the browser with mocked APIs that both nodes 
survive a save, and selection works after rename and direct-link reload.



##########
web-v2/web/src/app/catalogs/rightContent/CreateSemanticModelDialog.js:
##########
@@ -0,0 +1,266 @@
+/*
+ * 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.
+ */
+
+'use client'
+
+import React, { useEffect, useRef, useState } from 'react'
+import { Form, Input, Modal, Spin, Typography } from 'antd'
+import { useScrolling } from 'react-use'
+import RenderPropertiesFormItem from '@/components/EntityPropertiesFormItem'
+import { validateMessages, dialogContentMaxHeigth, mismatchName } from 
'@/config'
+import { nameRegex } from '@/lib/utils/regex'
+import { useResetFormOnCloseModal } from '@/lib/hooks/use-reset'
+import { cn } from '@/lib/utils/tailwind'
+import { useAppDispatch } from '@/lib/hooks/useStore'
+import {
+  formatSemanticModelDefinition,
+  genSemanticModelUpdates,
+  parseSemanticModelDefinition
+} from '@/lib/utils/semanticModel'
+import { createSemanticModel, updateSemanticModel, getSemanticModelDetails } 
from '@/lib/store/metalakes'
+
+const { Paragraph } = Typography
+const { TextArea } = Input
+
+const definitionPlaceholder = `{
+  "datasets": [
+    {
+      "name": "orders",
+      "source": { "namespace": ["sales", "mart"], "name": "orders" },
+      "fields": []
+    }
+  ]
+}`
+
+const defaultValues = {
+  name: '',
+  comment: '',
+  definition: '',
+  properties: []
+}
+
+export default function CreateSemanticModelDialog({ ...props }) {
+  const { open, setOpen, metalake, catalog, schema, editSemanticModel } = props
+  const [confirmLoading, setConfirmLoading] = useState(false)
+  const [isLoading, setIsLoading] = useState(false)
+  const [cacheData, setCacheData] = useState()
+  const scrollRef = useRef(null)
+  const loadedRef = useRef(false)
+  const scrolling = useScrolling(scrollRef)
+  const [bottomShadow, setBottomShadow] = useState(false)
+  const [topShadow, setTopShadow] = useState(false)
+  const [form] = Form.useForm()
+  const values = Form.useWatch([], form)
+  const dispatch = useAppDispatch()
+
+  useResetFormOnCloseModal({
+    form,
+    open
+  })
+
+  const handScroll = () => {
+    if (scrollRef.current) {
+      const { scrollTop, scrollHeight, clientHeight } = scrollRef.current
+      if (scrollHeight > clientHeight + scrollTop) {
+        setTopShadow(true)
+        setBottomShadow(scrollTop > 0)
+      } else if (scrollHeight === clientHeight + scrollTop) {
+        setTopShadow(false)
+        setBottomShadow(scrollTop > 0)
+      }
+    }
+  }
+
+  useEffect(() => {
+    scrollRef.current && handScroll()
+  }, [scrolling])
+
+  useEffect(() => {
+    if (open && editSemanticModel && !loadedRef.current) {
+      loadedRef.current = true
+
+      const initLoad = async () => {
+        setIsLoading(true)
+        try {
+          const { payload } = await dispatch(
+            getSemanticModelDetails({ metalake, catalog, schema, 
semanticModel: editSemanticModel })
+          )
+          const loaded = payload?.semanticModel
+          setCacheData(loaded)
+          form.setFieldValue('name', loaded?.name)
+          form.setFieldValue('comment', loaded?.comment)
+          form.setFieldValue('definition', 
formatSemanticModelDefinition(loaded?.definition))
+          let index = 0
+          Object.entries(loaded?.properties || {}).forEach(([key, value]) => {
+            form.setFieldValue(['properties', index, 'key'], key)
+            form.setFieldValue(['properties', index, 'value'], value)
+            index++
+          })
+          setIsLoading(false)
+        } catch (e) {
+          setIsLoading(false)
+        }
+      }
+      initLoad()
+    }
+
+    // Reset loadedRef when dialog closes
+    if (!open) {
+      loadedRef.current = false
+    }
+  }, [open, editSemanticModel, metalake, catalog, schema])
+
+  const validateDefinition = (rule, value) => {
+    const { errors } = parseSemanticModelDefinition(value)
+
+    return errors.length ? Promise.reject(new Error(errors.join(' '))) : 
Promise.resolve()
+  }
+
+  const handleSubmit = e => {
+    e.preventDefault()
+    form
+      .validateFields()
+      .then(async () => {
+        const { definition } = parseSemanticModelDefinition(values.definition)
+
+        setConfirmLoading(true)
+
+        const submitData = {
+          name: values.name.trim(),
+          comment: values.comment,
+          definition,
+          properties: (values.properties || []).reduce((acc, item) => {
+            acc[item.key] = values[item.key] || item.value
+
+            return acc
+          }, {})
+        }
+
+        if (editSemanticModel) {
+          const updates = genSemanticModelUpdates(cacheData, submitData)
+          if (updates.length) {
+            await dispatch(
+              updateSemanticModel({
+                metalake,
+                catalog,
+                schema,
+                semanticModel: editSemanticModel,
+                data: { updates }
+              })
+            )
+          }
+        } else {
+          await dispatch(createSemanticModel({ data: submitData, metalake, 
catalog, schema }))
+        }
+
+        setConfirmLoading(false)
+        setOpen(false)

Review Comment:
   Fixed in 6f82c2c86. The dialog checks the action result and closes only on 
success; `confirmLoading` resets in `finally`. Added component tests for failed 
creates and edits preserving input and allowing a successful retry. Also 
reproduced and verified both HTTP 400 paths in the browser with mocked APIs.



##########
web-v2/web/src/app/catalogs/rightContent/CreateSemanticModelDialog.js:
##########
@@ -0,0 +1,266 @@
+/*
+ * 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.
+ */
+
+'use client'
+
+import React, { useEffect, useRef, useState } from 'react'
+import { Form, Input, Modal, Spin, Typography } from 'antd'
+import { useScrolling } from 'react-use'
+import RenderPropertiesFormItem from '@/components/EntityPropertiesFormItem'
+import { validateMessages, dialogContentMaxHeigth, mismatchName } from 
'@/config'
+import { nameRegex } from '@/lib/utils/regex'
+import { useResetFormOnCloseModal } from '@/lib/hooks/use-reset'
+import { cn } from '@/lib/utils/tailwind'
+import { useAppDispatch } from '@/lib/hooks/useStore'
+import {
+  formatSemanticModelDefinition,
+  genSemanticModelUpdates,
+  parseSemanticModelDefinition
+} from '@/lib/utils/semanticModel'
+import { createSemanticModel, updateSemanticModel, getSemanticModelDetails } 
from '@/lib/store/metalakes'
+
+const { Paragraph } = Typography
+const { TextArea } = Input
+
+const definitionPlaceholder = `{
+  "datasets": [
+    {
+      "name": "orders",
+      "source": { "namespace": ["sales", "mart"], "name": "orders" },
+      "fields": []
+    }
+  ]
+}`
+
+const defaultValues = {
+  name: '',
+  comment: '',
+  definition: '',
+  properties: []
+}
+
+export default function CreateSemanticModelDialog({ ...props }) {
+  const { open, setOpen, metalake, catalog, schema, editSemanticModel } = props
+  const [confirmLoading, setConfirmLoading] = useState(false)
+  const [isLoading, setIsLoading] = useState(false)
+  const [cacheData, setCacheData] = useState()
+  const scrollRef = useRef(null)
+  const loadedRef = useRef(false)
+  const scrolling = useScrolling(scrollRef)
+  const [bottomShadow, setBottomShadow] = useState(false)
+  const [topShadow, setTopShadow] = useState(false)
+  const [form] = Form.useForm()
+  const values = Form.useWatch([], form)
+  const dispatch = useAppDispatch()
+
+  useResetFormOnCloseModal({
+    form,
+    open
+  })
+
+  const handScroll = () => {
+    if (scrollRef.current) {
+      const { scrollTop, scrollHeight, clientHeight } = scrollRef.current
+      if (scrollHeight > clientHeight + scrollTop) {
+        setTopShadow(true)
+        setBottomShadow(scrollTop > 0)
+      } else if (scrollHeight === clientHeight + scrollTop) {
+        setTopShadow(false)
+        setBottomShadow(scrollTop > 0)
+      }
+    }
+  }
+
+  useEffect(() => {
+    scrollRef.current && handScroll()
+  }, [scrolling])
+
+  useEffect(() => {
+    if (open && editSemanticModel && !loadedRef.current) {
+      loadedRef.current = true
+
+      const initLoad = async () => {
+        setIsLoading(true)
+        try {
+          const { payload } = await dispatch(
+            getSemanticModelDetails({ metalake, catalog, schema, 
semanticModel: editSemanticModel })
+          )
+          const loaded = payload?.semanticModel
+          setCacheData(loaded)
+          form.setFieldValue('name', loaded?.name)
+          form.setFieldValue('comment', loaded?.comment)
+          form.setFieldValue('definition', 
formatSemanticModelDefinition(loaded?.definition))
+          let index = 0
+          Object.entries(loaded?.properties || {}).forEach(([key, value]) => {
+            form.setFieldValue(['properties', index, 'key'], key)
+            form.setFieldValue(['properties', index, 'value'], value)
+            index++
+          })
+          setIsLoading(false)
+        } catch (e) {
+          setIsLoading(false)
+        }
+      }
+      initLoad()
+    }
+
+    // Reset loadedRef when dialog closes
+    if (!open) {
+      loadedRef.current = false
+    }
+  }, [open, editSemanticModel, metalake, catalog, schema])
+
+  const validateDefinition = (rule, value) => {
+    const { errors } = parseSemanticModelDefinition(value)
+
+    return errors.length ? Promise.reject(new Error(errors.join(' '))) : 
Promise.resolve()
+  }
+
+  const handleSubmit = e => {
+    e.preventDefault()
+    form
+      .validateFields()
+      .then(async () => {
+        const { definition } = parseSemanticModelDefinition(values.definition)
+
+        setConfirmLoading(true)
+
+        const submitData = {
+          name: values.name.trim(),
+          comment: values.comment,
+          definition,
+          properties: (values.properties || []).reduce((acc, item) => {
+            acc[item.key] = values[item.key] || item.value
+
+            return acc
+          }, {})
+        }

Review Comment:
   Fixed in 6f82c2c86. Properties are now built directly from each list item's 
`key` and `value` using `Object.fromEntries`, with no lookup in the top-level 
form or truthiness fallback. The component regression test covers properties 
named `name`, `comment`, and `definition`, including an empty-string value.



-- 
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]

Reply via email to