Copilot commented on code in PR #11654:
URL: https://github.com/apache/gravitino/pull/11654#discussion_r3417663471
##########
web-v2/web/src/app/catalogs/page.js:
##########
@@ -167,6 +167,7 @@ const CatalogsListPage = () => {
await dispatch(fetchCatalogs({ metalake }))
await dispatch(fetchSchemas({ metalake, catalog, catalogType }))
}
+ dispatch(fetchSchemas({ metalake, catalog, catalogType,
parentSchema: schema }))
Review Comment:
`fetchSchemas({ parentSchema: schema })` is dispatched for every schema
route, even though subschemas are only supported for specific catalog backends
(e.g., Iceberg JDBC). This adds an extra API call on every schema navigation
and can trigger avoidable errors if the backend doesn’t support `parentSchema`.
Gate this dispatch based on the catalog’s provider/backend.
##########
web-v2/web/src/app/catalogs/rightContent/entitiesContent/SchemaDetailsPage.js:
##########
@@ -308,24 +298,41 @@ export default function SchemaDetailsPage() {
}
const showDeleteSchemaConfirm = name => {
+ let confirmInput = ''
+ let validateFn = null
+
+ const setConfirmInput = value => {
+ confirmInput = value
+ }
+
+ const registerValidate = fn => {
+ validateFn = fn
+ }
+
modal.confirm({
title: `Are you sure to delete the schema ${name}?`,
icon: <ExclamationCircleFilled />,
+ content: <ConfirmInput name={name} setConfirmInput={setConfirmInput}
registerValidate={registerValidate} />,
okText: 'Delete',
okType: 'danger',
cancelText: 'Cancel',
- onOk: async () => {
- await dispatch(deleteSchema({ metalake: currentMetalake, catalog,
catalogType, schema: name }))
+ onOk(close) {
+ if (validateFn && !validateFn()) return
- const [err, res] = await to(
- dispatch(fetchSchemas({ metalake: currentMetalake, catalog,
catalogType, parentSchema: schema }))
- )
- if (!err && res) {
- const { schemas = [] } = res.payload || {}
- const nextSubSchemas = schemas.map(item => ({ ...item, name:
item.name, key: item.name, title: item.name }))
- setSubSchemas(nextSubSchemas)
+ const confirmFn = async () => {
+ await dispatch(deleteSchema({ metalake: currentMetalake, catalog,
catalogType, schema: name }))
+ console.log(`reloading schema ${schema}, reloading subschemas...`)
+
+ // Fetch subschemas again to update store.subschemas
+ await dispatch(fetchSchemas({ metalake: currentMetalake, catalog,
catalogType, parentSchema: schema }))
+
+ treeRef.current.onLoadData(
+ { key:
`{{${currentMetalake}}}{{${catalog}}}{{${catalogType}}}{{${schema}}}`,
nodeType: 'schema' },
+ true
+ )
+ close()
}
- treeRef.current.onLoadData({ key: catalog, nodeType: 'catalog', inUse:
'true' }, true)
+ confirmFn()
}
Review Comment:
The delete-schema confirm handler starts an async flow but doesn’t
return/await it, and it also logs to the console. This can cause the modal to
close before the deletion/refresh completes (depending on Modal behavior) and
leaves noisy debug output in production. Make `onOk` async (so AntD can track
loading) and remove the `console.log`.
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -2706,9 +2713,13 @@ export const appMetalakesSlice = createSlice({
}
})
builder.addCase(setIntoTreeNodeWithFetch.fulfilled, (state, action) => {
- const { key, data } = action.payload
+ const { key, data, entities } = action.payload
state.metalakeTree = updateTreeData(state.metalakeTree, key, data)
+
+ if (entities) {
+ state.tableData = entities
+ }
Review Comment:
`setIntoTreeNodeWithFetch.fulfilled` is now overwriting `state.tableData`
whenever a schema node is lazily loaded/expanded in the tree. This can
unexpectedly change the right-side list (e.g., while viewing a catalog’s schema
list) just by expanding a node. `tableData` should only be updated when the
loaded node is currently selected (or otherwise explicitly requested by the
page), not on every tree load.
##########
web-v2/web/src/app/catalogs/rightContent/entitiesContent/SchemaDetailsPage.js:
##########
@@ -308,24 +298,41 @@ export default function SchemaDetailsPage() {
}
const showDeleteSchemaConfirm = name => {
+ let confirmInput = ''
+ let validateFn = null
+
+ const setConfirmInput = value => {
+ confirmInput = value
+ }
+
+ const registerValidate = fn => {
+ validateFn = fn
+ }
Review Comment:
`confirmInput` is written via `setConfirmInput` but never read; the parent
component doesn’t need to mirror this state because `ConfirmInput` already
validates internally via `registerValidate`. This extra local variable adds
noise and can be removed.
--
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]
