Copilot commented on code in PR #9994:
URL: https://github.com/apache/gravitino/pull/9994#discussion_r2802170537
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -1769,6 +1767,26 @@ export const appMetalakesSlice = createSlice({
const { key, data } = action.payload
state.metalakeTree = updateTreeData(state.metalakeTree, key, data)
},
+ updateCatalogInUse(state, action) {
+ const { catalog, isInUse } = action.payload
+ const inUseValue = isInUse ? 'true' : 'false'
+ state.tableData = state.tableData.map(item => {
+ if (item.name !== catalog) return item
+
+ return {
+ ...item,
+ properties: { ...(item.properties || {}), 'in-use': inUseValue }
+ }
+ })
+ state.catalogs = state.catalogs.map(item => {
+ if (item.name !== catalog) return item
+
+ return {
+ ...item,
+ properties: { ...(item.properties || {}), 'in-use': inUseValue }
+ }
+ })
Review Comment:
`updateCatalogInUse` matches catalogs by `name` only. Elsewhere in this
slice catalog identity includes both `name` and `type` (e.g., keys like
`{{metalake}}{{name}}{{type}}`), so two catalogs with the same name but
different types would both be updated incorrectly. Include `catalogType` (or
the full key) in the action payload and update only the matching entry.
##########
web-v2/web/src/app/catalogs/rightContent/entitiesContent/CatalogsPage.js:
##########
@@ -325,11 +342,13 @@ export default function CatalogsPage() {
const handleChangeInUse = async (catalogObj, checked) => {
await dispatch(switchInUseCatalog({ metalake: currentMetalake, catalog:
catalogObj.name, isInUse: checked }))
Review Comment:
`handleChangeInUse` awaits `dispatch(switchInUseCatalog(...))`, but
`dispatch` resolves to an action even on thunk rejection (it won’t throw). As a
result, `updateCatalogInUse` (and the tree update) will run even if the PATCH
request failed, leaving the UI in an incorrect optimistic state and potentially
enabling a delete that the backend still considers “in-use”. Use
`dispatch(switchInUseCatalog(...)).unwrap()` (and catch to rollback UI), or
check `action.meta.requestStatus` before updating local state.
```suggestion
const action = await dispatch(
switchInUseCatalog({ metalake: currentMetalake, catalog:
catalogObj.name, isInUse: checked })
)
if (!action?.meta || action.meta.requestStatus !== 'fulfilled') {
return
}
```
##########
web-v2/web/src/app/catalogs/rightContent/entitiesContent/CatalogsPage.js:
##########
@@ -140,25 +132,56 @@ export default function CatalogsPage() {
validateFn = fn
}
- modal.confirm({
- title: `Are you sure to delete the ${type} ${catalog.name}?`,
- icon: <ExclamationCircleFilled />,
- content: (
- <NameContext.Consumer>
- {name => (
+ const updateInUse = value => {
+ currentInUse = value
+ }
+
+ let confirmModal
+
+ const DeleteCatalogConfirmContent = () => {
+ const [inUse, setInUse] = useState(currentInUse)
+
+ const handleSwitchChange = async checked => {
+ setInUse(checked)
+ updateInUse(checked)
+ confirmModal?.update({ okButtonProps: { disabled: checked } })
+ await handleChangeInUse(catalog, checked)
+ }
+
+ return (
+ <Flex vertical gap={8}>
+ {initialInUse ? (
+ <>
+ <Paragraph style={{ marginBottom: 0 }}>
+ Please set the {type} to "Not In-Use" before
deleting.
+ </Paragraph>
+ <Flex align='center' gap={8}>
+ <Switch size='small' checked={inUse}
onChange={handleSwitchChange} />
+ <span>{inUse ? 'In-Use' : 'Not In-Use'}</span>
+ </Flex>
+ </>
+ ) : (
<ConfirmInput
- name={name}
+ name={catalog.name}
type={type}
setConfirmInput={setConfirmInput}
registerValidate={registerValidate}
/>
)}
Review Comment:
The delete confirm content is gated by `initialInUse`. If the catalog starts
"in-use", after switching it to "Not In-Use" in this dialog the user can delete
without the usual name-typing confirmation (`ConfirmInput` never renders). If
the intent is to keep the same safety check as other delete flows, render
`ConfirmInput` once `inUse` becomes false (and keep the OK button disabled
until both: not in-use + confirmation passes).
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -695,8 +695,6 @@ export const switchInUseCatalog = createAsyncThunk(
throw new Error(err)
}
Review Comment:
`switchInUseCatalog` no longer refreshes Redux state (`fetchCatalogs`
dispatch was removed), but the slice also doesn’t update `state.metalakeTree`
on switch. Since `metalakeTree` is only initialized from `fetchCatalogs` when
empty, the in-use change won’t persist in the global store and can revert on
navigation/remount or other updates. Consider either restoring the
`fetchCatalogs({ metalake, init: true })` dispatch here, or add a
`switchInUseCatalog.fulfilled` handler that updates
`catalogs/tableData/metalakeTree` consistently.
```suggestion
// Refresh catalogs and metalake tree to persist in-use state in the
global store
dispatch(fetchCatalogs({ metalake, catalog, page: 'metalakes', init:
true }))
```
--
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]
