Copilot commented on code in PR #10114:
URL: https://github.com/apache/gravitino/pull/10114#discussion_r2871047504
##########
web-v2/web/src/app/catalogs/TreeComponent.js:
##########
@@ -323,29 +295,58 @@ export const TreeComponent = forwardRef(function
TreeComponent(props, ref) {
if (searchParams.get('catalogType')) {
setCatalogType(searchParams.get('catalogType'))
} else {
- setExpandedKeys([])
+ dispatch(setExpanded([]))
setCatalogType('relational')
}
- }, [searchParams.get('catalogType')])
+ }, [dispatch, searchParams])
useEffect(() => {
setTimeout(() => {
- if (entity) {
- setExpandedKeys(
- Array.from(new Set([...expandedKeys, catalog,
`${catalog}/${schema}`, `${catalog}/${schema}/${entity}`]))
- )
- setSelectedKeys([`${catalog}/${schema}/${entity}`])
- } else if (schema && !entity) {
- setExpandedKeys(Array.from(new Set([...expandedKeys, catalog,
`${catalog}/${schema}`])))
- setSelectedKeys([`${catalog}/${schema}`])
- } else if (catalog && !schema && !entity) {
- setExpandedKeys(Array.from(new Set([...expandedKeys, catalog])))
- setSelectedKeys([catalog])
- } else if (!catalog && !schema && !entity) {
- setExpandedKeys(Array.from(new Set([...expandedKeys])))
+ const metalake = searchParams.get('metalake')
+ const catalog = searchParams.get('catalog')
+ const catalogType = searchParams.get('catalogType')
+ const schema = searchParams.get('schema')
+ const table = searchParams.get('table')
+ const fileset = searchParams.get('fileset')
+ const topic = searchParams.get('topic')
+ const model = searchParams.get('model')
+ const func = searchParams.get('function')
+ const entity = table || fileset || topic || model || func
+
+ if (!metalake) {
+ dispatch(setExpandedNodes([]))
Review Comment:
`setExpandedNodes` merges the provided keys into the existing expandedNodes
(it doesn’t replace/clear). Dispatching `setExpandedNodes([])` therefore won’t
clear expanded state when `metalake` is missing, and stale expanded keys can
remain. Use `setExpanded([])` (or a dedicated reset action) here if the intent
is to clear.
```suggestion
dispatch(setExpanded([]))
```
##########
web-v2/web/src/app/catalogs/TreeComponent.js:
##########
@@ -323,29 +295,58 @@ export const TreeComponent = forwardRef(function
TreeComponent(props, ref) {
if (searchParams.get('catalogType')) {
setCatalogType(searchParams.get('catalogType'))
} else {
- setExpandedKeys([])
+ dispatch(setExpanded([]))
setCatalogType('relational')
}
- }, [searchParams.get('catalogType')])
+ }, [dispatch, searchParams])
useEffect(() => {
setTimeout(() => {
- if (entity) {
- setExpandedKeys(
- Array.from(new Set([...expandedKeys, catalog,
`${catalog}/${schema}`, `${catalog}/${schema}/${entity}`]))
- )
- setSelectedKeys([`${catalog}/${schema}/${entity}`])
- } else if (schema && !entity) {
- setExpandedKeys(Array.from(new Set([...expandedKeys, catalog,
`${catalog}/${schema}`])))
- setSelectedKeys([`${catalog}/${schema}`])
- } else if (catalog && !schema && !entity) {
- setExpandedKeys(Array.from(new Set([...expandedKeys, catalog])))
- setSelectedKeys([catalog])
- } else if (!catalog && !schema && !entity) {
- setExpandedKeys(Array.from(new Set([...expandedKeys])))
+ const metalake = searchParams.get('metalake')
+ const catalog = searchParams.get('catalog')
+ const catalogType = searchParams.get('catalogType')
+ const schema = searchParams.get('schema')
+ const table = searchParams.get('table')
+ const fileset = searchParams.get('fileset')
+ const topic = searchParams.get('topic')
+ const model = searchParams.get('model')
+ const func = searchParams.get('function')
+ const entity = table || fileset || topic || model || func
+
+ if (!metalake) {
+ dispatch(setExpandedNodes([]))
+ dispatch(setSelectedNodes([]))
+
+ return
+ }
+
+ const metalakeKey = `{{${metalake}}}`
+ const nextExpandedKeys = [metalakeKey]
+ let nextSelectedKey = ''
+
+ if (catalog && catalogType) {
+ const catalogKey = `${metalakeKey}{{${catalog}}}{{${catalogType}}}`
+ nextExpandedKeys.push(catalogKey)
+ nextSelectedKey = catalogKey
+
+ if (schema) {
+ const schemaKey = `${catalogKey}{{${schema}}}`
+ nextExpandedKeys.push(schemaKey)
+ nextSelectedKey = schemaKey
+
+ if (entity) {
+ nextSelectedKey = `${schemaKey}{{${entity}}}`
+ }
+ }
+ }
+
+ dispatch(setExpandedNodes(Array.from(new Set(nextExpandedKeys))))
+
+ if (nextSelectedKey) {
+ dispatch(setSelectedNodes([nextSelectedKey]))
Review Comment:
Here the code appears to be computing the *exact* expanded path based on the
current route, but it dispatches `setExpandedNodes(...)` which only merges/adds
keys and never removes old ones. This can leave unrelated nodes
expanded/selected and may contribute to the “cursor stays at schema level”
behavior when switching entities. If you want the expanded state to match
`nextExpandedKeys` exactly, dispatch `setExpanded(nextExpandedKeys)` instead.
##########
web-v2/web/src/app/catalogs/TreeComponent.js:
##########
@@ -323,29 +295,58 @@ export const TreeComponent = forwardRef(function
TreeComponent(props, ref) {
if (searchParams.get('catalogType')) {
setCatalogType(searchParams.get('catalogType'))
} else {
- setExpandedKeys([])
+ dispatch(setExpanded([]))
setCatalogType('relational')
}
- }, [searchParams.get('catalogType')])
+ }, [dispatch, searchParams])
useEffect(() => {
setTimeout(() => {
- if (entity) {
- setExpandedKeys(
- Array.from(new Set([...expandedKeys, catalog,
`${catalog}/${schema}`, `${catalog}/${schema}/${entity}`]))
- )
- setSelectedKeys([`${catalog}/${schema}/${entity}`])
- } else if (schema && !entity) {
- setExpandedKeys(Array.from(new Set([...expandedKeys, catalog,
`${catalog}/${schema}`])))
- setSelectedKeys([`${catalog}/${schema}`])
- } else if (catalog && !schema && !entity) {
- setExpandedKeys(Array.from(new Set([...expandedKeys, catalog])))
- setSelectedKeys([catalog])
- } else if (!catalog && !schema && !entity) {
- setExpandedKeys(Array.from(new Set([...expandedKeys])))
+ const metalake = searchParams.get('metalake')
Review Comment:
The effect schedules a setTimeout but never clears it. If the component
unmounts or the search params change quickly, the callback can still dispatch
after unmount / out of date, causing hard-to-reproduce UI state bugs. Store the
timer id and clearTimeout it in the effect cleanup (and consider avoiding the
artificial delay if not strictly required).
##########
web-v2/web/src/app/catalogs/TreeComponent.js:
##########
@@ -493,12 +494,16 @@ export const TreeComponent = forwardRef(function
TreeComponent(props, ref) {
}
}
if (inUseStr === 'false') {
+ const catalogPrefix =
`{{${currentMetalake}}}{{${name}}}{{${currentCatalogType}}}`
+
// Remove from expanded and loaded nodes
- dispatch(setExpandedNodes([...store.expandedNodes.filter(key =>
!key.includes(name))]))
- dispatch(setLoadedNodes([...store.loadedNodes.filter(key =>
!key.includes(name))]))
+ dispatch(setExpandedNodes([...store.expandedNodes.filter(key =>
!key.startsWith(catalogPrefix))]))
Review Comment:
This is trying to *remove* keys under a catalog from `expandedNodes`, but it
uses `setExpandedNodes(...)` which merges with the existing expandedNodes and
won’t remove anything. Use `setExpanded(filteredExpandedKeys)` (replace
semantics) to actually drop the removed prefixes.
```suggestion
dispatch(setExpanded([...store.expandedNodes.filter(key =>
!key.startsWith(catalogPrefix))]))
```
##########
web-v2/web/src/app/catalogs/page.js:
##########
@@ -171,115 +184,123 @@ const CatalogsListPage = () => {
)
}
- if (paramsSize === 5 && catalog && catalogType && schema && (table ||
fileset || topic || model)) {
+ if (paramsSize === 5 && catalog && catalogType && schema) {
if (!store.catalogs.length) {
await dispatch(fetchCatalogs({ metalake }))
await dispatch(fetchSchemas({ metalake, catalog, catalogType }))
}
- switch (catalogType) {
- case 'relational':
- store.tables.length === 0 &&
- (await dispatch(fetchTables({ init: true, page: 'schemas',
metalake, catalog, schema })))
- await dispatch(resetActivatedDetails())
- await dispatch(setActivatedDetailsLoading(true))
- await dispatch(getTableDetails({ init: true, metalake, catalog,
schema, table }))
- await dispatch(setActivatedDetailsLoading(false))
- dispatch(
- getCurrentEntityTags({
- init: true,
- metalake,
- metadataObjectType: 'table',
- metadataObjectFullName: `${catalog}.${schema}.${table}`
- })
- )
- dispatch(
- getCurrentEntityPolicies({
- init: true,
- metalake,
- metadataObjectType: 'table',
- metadataObjectFullName: `${catalog}.${schema}.${table}`,
- details: true
- })
- )
- break
- case 'fileset':
- store.filesets.length === 0 &&
- (await dispatch(fetchFilesets({ init: true, page: 'schemas',
metalake, catalog, schema })))
- await dispatch(setActivatedDetailsLoading(true))
- await dispatch(getFilesetDetails({ init: true, metalake,
catalog, schema, fileset }))
- await dispatch(setActivatedDetailsLoading(false))
- dispatch(
- getCurrentEntityTags({
- init: true,
- metalake,
- metadataObjectType: 'fileset',
- metadataObjectFullName: `${catalog}.${schema}.${fileset}`,
- details: true
- })
- )
- dispatch(
- getCurrentEntityPolicies({
- init: true,
- metalake,
- metadataObjectType: 'fileset',
- metadataObjectFullName: `${catalog}.${schema}.${fileset}`,
- details: true
- })
- )
- break
- case 'messaging':
- store.topics.length === 0 &&
- (await dispatch(fetchTopics({ init: true, page: 'schemas',
metalake, catalog, schema })))
- await dispatch(setActivatedDetailsLoading(true))
- await dispatch(getTopicDetails({ init: true, metalake, catalog,
schema, topic }))
- await dispatch(setActivatedDetailsLoading(false))
- dispatch(
- getCurrentEntityTags({
- init: true,
- metalake,
- metadataObjectType: 'topic',
- metadataObjectFullName: `${catalog}.${schema}.${topic}`,
- details: true
- })
- )
- dispatch(
- getCurrentEntityPolicies({
- init: true,
- metalake,
- metadataObjectType: 'topic',
- metadataObjectFullName: `${catalog}.${schema}.${topic}`,
- details: true
- })
- )
- break
- case 'model':
- store.models.length === 0 &&
- (await dispatch(fetchModels({ init: true, page: 'schemas',
metalake, catalog, schema })))
- await dispatch(fetchModelVersions({ init: true, metalake,
catalog, schema, model }))
- await dispatch(setActivatedDetailsLoading(true))
- await dispatch(getModelDetails({ init: true, metalake, catalog,
schema, model }))
- await dispatch(setActivatedDetailsLoading(false))
- dispatch(
- getCurrentEntityTags({
- init: true,
- metalake,
- metadataObjectType: 'model',
- metadataObjectFullName: `${catalog}.${schema}.${model}`,
- details: true
- })
- )
- dispatch(
- getCurrentEntityPolicies({
- init: true,
- metalake,
- metadataObjectType: 'model',
- metadataObjectFullName: `${catalog}.${schema}.${model}`,
- details: true
- })
- )
- break
- default:
- break
+ if (table || fileset || topic || model) {
+ switch (catalogType) {
+ case 'relational':
+ store.tables.length === 0 &&
+ (await dispatch(fetchTables({ init: true, page: 'schemas',
metalake, catalog, schema })))
+ await dispatch(resetActivatedDetails())
+ await dispatch(setActivatedDetailsLoading(true))
+ await dispatch(getTableDetails({ init: true, metalake,
catalog, schema, table }))
+ await dispatch(setActivatedDetailsLoading(false))
+ dispatch(
+ getCurrentEntityTags({
+ init: true,
+ metalake,
+ metadataObjectType: 'table',
+ metadataObjectFullName: `${catalog}.${schema}.${table}`
+ })
+ )
+ dispatch(
+ getCurrentEntityPolicies({
+ init: true,
+ metalake,
+ metadataObjectType: 'table',
+ metadataObjectFullName: `${catalog}.${schema}.${table}`,
+ details: true
+ })
+ )
+ break
+ case 'fileset':
+ store.filesets.length === 0 &&
+ (await dispatch(fetchFilesets({ init: true, page: 'schemas',
metalake, catalog, schema })))
+ await dispatch(setActivatedDetailsLoading(true))
+ await dispatch(getFilesetDetails({ init: true, metalake,
catalog, schema, fileset }))
+ await dispatch(setActivatedDetailsLoading(false))
+ dispatch(
+ getCurrentEntityTags({
+ init: true,
+ metalake,
+ metadataObjectType: 'fileset',
+ metadataObjectFullName: `${catalog}.${schema}.${fileset}`,
+ details: true
+ })
+ )
+ dispatch(
+ getCurrentEntityPolicies({
+ init: true,
+ metalake,
+ metadataObjectType: 'fileset',
+ metadataObjectFullName: `${catalog}.${schema}.${fileset}`,
+ details: true
+ })
+ )
+ break
+ case 'messaging':
+ store.topics.length === 0 &&
+ (await dispatch(fetchTopics({ init: true, page: 'schemas',
metalake, catalog, schema })))
+ await dispatch(setActivatedDetailsLoading(true))
+ await dispatch(getTopicDetails({ init: true, metalake,
catalog, schema, topic }))
+ await dispatch(setActivatedDetailsLoading(false))
+ dispatch(
+ getCurrentEntityTags({
+ init: true,
+ metalake,
+ metadataObjectType: 'topic',
+ metadataObjectFullName: `${catalog}.${schema}.${topic}`,
+ details: true
+ })
+ )
+ dispatch(
+ getCurrentEntityPolicies({
+ init: true,
+ metalake,
+ metadataObjectType: 'topic',
+ metadataObjectFullName: `${catalog}.${schema}.${topic}`,
+ details: true
+ })
+ )
+ break
+ case 'model':
+ store.models.length === 0 &&
+ (await dispatch(fetchModels({ init: true, page: 'schemas',
metalake, catalog, schema })))
+ await dispatch(fetchModelVersions({ init: true, metalake,
catalog, schema, model }))
+ await dispatch(setActivatedDetailsLoading(true))
+ await dispatch(getModelDetails({ init: true, metalake,
catalog, schema, model }))
+ await dispatch(setActivatedDetailsLoading(false))
+ dispatch(
+ getCurrentEntityTags({
+ init: true,
+ metalake,
+ metadataObjectType: 'model',
+ metadataObjectFullName: `${catalog}.${schema}.${model}`,
+ details: true
+ })
+ )
+ dispatch(
+ getCurrentEntityPolicies({
+ init: true,
+ metalake,
+ metadataObjectType: 'model',
+ metadataObjectFullName: `${catalog}.${schema}.${model}`,
+ details: true
+ })
+ )
+ break
+ default:
+ break
+ }
+ }
+ if (func) {
+ store.functions.length === 0 && (await dispatch(fetchFunctions({
init: true, metalake, catalog, schema })))
Review Comment:
When loading a function detail you set the loading flag but don’t clear
`activatedDetails` first. If `getFunctionDetails` is rejected, the UI can keep
showing the previous entity’s details (stale state) after loading ends.
Consider dispatching `resetActivatedDetails()` before setting
`activatedDetailsLoading(true)` here (matching the table branch).
```suggestion
store.functions.length === 0 && (await dispatch(fetchFunctions({
init: true, metalake, catalog, schema })))
dispatch(resetActivatedDetails())
```
--
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]
