jerryshao commented on code in PR #12900:
URL: https://github.com/apache/gravitino/pull/12900#discussion_r4290193966
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -3041,68 +3270,87 @@ export const appMetalakesSlice = createSlice({
}
})
builder.addCase(fetchModelVersions.rejected, (state, action) => {
- if (!action.error.message.includes('CanceledError')) {
+ if (!action.meta.aborted &&
!action.error.message.includes('CanceledError')) {
toast.error(action.error.message)
}
})
builder.addCase(fetchFunctions.fulfilled, (state, action) => {
state.functions = action.payload.functions
})
- builder.addCase(getFunctionDetails.fulfilled, (state, action) => {
- if (action.payload.init) {
- state.activatedDetails = action.payload.function
- }
- })
builder.addCase(fetchFunctions.rejected, (state, action) => {
- if (!action.error.message.includes('CanceledError')) {
- toast.error(action.error.message)
- }
- })
- builder.addCase(getFunctionDetails.rejected, (state, action) => {
- if (!action.error.message.includes('CanceledError')) {
+ if (!action.meta.aborted &&
!action.error.message.includes('CanceledError')) {
toast.error(action.error.message)
}
})
builder.addCase(fetchViews.fulfilled, (state, action) => {
state.views = action.payload.views
})
builder.addCase(fetchViews.rejected, (state, action) => {
- if (!action.error.message.includes('CanceledError')) {
+ if (!action.meta.aborted &&
!action.error.message.includes('CanceledError')) {
toast.error(action.error.message)
}
})
- builder.addCase(getViewDetails.fulfilled, (state, action) => {
- if (action.payload.init) {
- state.activatedDetails = action.payload.view
+ builder.addCase(deleteView.rejected, (state, action) => {
+ if (!action.meta.aborted &&
!action.error.message.includes('CanceledError')) {
+ toast.error(action.error.message)
+ }
+ })
+ builder.addCase(fetchSemanticModels.pending, (state, action) => {
+ const key = semanticModelSchemaKey(action.meta.arg)
+ trackSemanticModelRequest(state, key, action.meta.requestId, true)
+ if (!state.activeCatalogEntity || state.activeSemanticModelSchema ===
key)
+ state.semanticModelListRequestId = action.meta.requestId
+ })
+ builder.addCase(fetchSemanticModels.fulfilled, (state, action) => {
+ const key = semanticModelSchemaKey(action.meta.arg)
+ const request = state.semanticModelRequests[key]
+ if (!action.payload.stale && request?.requestId ===
action.meta.requestId) {
+ const data = mergeWithSemanticModelNodes({
+ tree: state.metalakeTree,
+ key,
+ entities: action.payload.semanticModels
+ })
+ state.metalakeTree = updateTreeData(state.metalakeTree, key, data)
Review Comment:
[Important] Moving the tree merge out of the thunk into this reducer dropped
the guard the thunk had at 6a36893 (`init &&
getState().metalakes.loadedNodes.includes(schemaKey)`), so the merge now also
runs for schema nodes whose children have never been fetched.
`updateTreeData` sets `isLeaf = children?.length === 0`
(lib/utils/index.js:311), so a relational schema that has tables but no
Semantic Models is rewritten to `{children: [], isLeaf: true}`. antd then
renders it as a leaf: the switcher disappears and `loadData` is never called,
so the schema's tables stay unreachable in the tree until the catalog is
reloaded. With models present it is milder but still wrong - the node briefly
shows the models as its only children, because `mergeWithSemanticModelNodes`
can only preserve table/view/function/sub-schema children that are already in
the tree.
Reachable path: expand a relational catalog, then click a schema that has
tables and no Semantic Models. `onSelect` only navigates
(TreeComponent.js:522-531), so the node is not in `loadedNodes`; `page.js:212`
dispatches `fetchSemanticModels({init: true})` and this reducer marks the node
a leaf well before the debounced `setExpandedNodes` at TreeComponent.js:392 (1s
timer) could trigger `loadData`.
Suggestion: restore the loaded-node condition for the `updateTreeData` call
(the `state.semanticModels` assignment below does not need it), or pass the
merge result only when the node already has children and otherwise leave
`isLeaf` untouched.
Verified by: running a reducer test on this checkout - seed `metalakeTree`
with a catalog whose child is `{key: '{{demo}}{{sales}}{{relational}}{{mart}}',
node: 'schema', children: []}` with empty `loadedNodes`, then dispatch
`fetchSemanticModels({init: true, ...})` against a `{semanticModels: []}`
response. On f82d2dd the node becomes `{"children":[],"isLeaf":true}`; the
identical test against 6a36893's `index.js` leaves it `{"children":[]}`.
##########
web-v2/web/src/lib/store/metalakes/index.js:
##########
@@ -2364,6 +2535,25 @@ export const appMetalakesSlice = createSlice({
currentEntityPolicies: []
},
reducers: {
+ setActiveCatalogEntity(state, action) {
+ const key = catalogEntityKey(action.payload)
+ const schemaKey = action.payload.schema ?
semanticModelSchemaKey(action.payload) : null
+ if (state.activeSemanticModelSchema !== schemaKey) state.semanticModels
= []
+ state.activeSemanticModelSchema = schemaKey
+ if (state.activeCatalogEntity === key) return
+ state.activeCatalogEntity = key
+ state.activatedDetails = null
Review Comment:
[Question] Should `getVersionDetails` be part of this guard as well?
This reducer clears `activatedDetails` whenever the entity key changes, and
`catalogEntityKey` treats `version` as its own kind (catalogEntity.js:21-43),
so opening a model version produces a new key and nulls the details. But
`getVersionDetails` is the one detail loader left out of the `detailThunks`
table (index.js:2935-2946) and it has no `fulfilled` handler at all
(index.js:3337 only handles `rejected`), while `page.js:402` dispatches it and
discards the payload.
On a version URL, `RightContent.js:144-153` still renders
`ModelDetailsPage`, which reads `store.activatedDetails?.comment` and
`?.properties` (ModelDetailsPage.js:133, 289-292). Before this commit those
values survived the model -> version navigation because nothing reset them; now
the header renders blank with the spinner already finished. A direct load of a
version URL was blank before this commit too, so this is a pre-existing gap
that the new reset makes visible on the navigation path.
If versions are meant to keep showing their model's header, either add
`[getModelDetails, 'model']`-style handling for `getVersionDetails` or have
`page.js` load the model's details for the version route; if the version route
is expected to be self-contained, it would be worth saying so, since it is now
the only detail route whose `activatedDetails` is cleared and never refilled.
Verified by: reading the reducer here, `catalogEntityKey`, the
`detailThunks` table and every `getVersionDetails` case in index.js (no
`fulfilled`), plus `page.js:394-405`, `RightContent.js:144-153` and
`ModelDetailsPage.js:133/289-292` in this checkout. Not reproduced in a browser.
--
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]
