Copilot commented on code in PR #12292:
URL: https://github.com/apache/gravitino/pull/12292#discussion_r3688716250
##########
web-v2/web/src/app/catalogs/rightContent/entitiesContent/ListFiles.js:
##########
@@ -149,27 +205,14 @@ const ListFiles = ({ metalake, catalog, schema, fileset,
storageLocations, defau
return { columns, minWidth: 100 }
}, [columns])
- if (!storageLocations || Object.keys(storageLocations).length === 0) {
- return <p>No storage locations configured</p>
- }
-
- if (!currentLocation && defaultLocationName) {
- setCurrentLocation(defaultLocationName)
-
+ if (!storageLocations || Object.keys(storageLocations).length === 0 ||
storageLocationsIsStale) {
return <Spin />
}
Review Comment:
The component now shows a loading spinner when storageLocations is empty. If
a fileset has no storage locations configured, this becomes an infinite spinner
with no explanation (regression from the previous “No storage locations
configured” message). Consider showing the message for the empty-but-not-stale
case and reserving <Spin/> for loading/stale props.
##########
web-v2/web/src/app/catalogs/rightContent/entitiesContent/ListFiles.js:
##########
@@ -41,11 +41,59 @@ const ListFiles = ({ metalake, catalog, schema, fileset,
storageLocations, defau
const store = useAppSelector(state => state.metalakes)
const dispatch = useAppDispatch()
+ // Track the current fileset identity to detect switches.
+ // Reset navigation state immediately during render (before useEffects run)
+ // so that no stale state can trigger API calls with wrong parameters.
+ const filesetIdentity = `${catalog}.${schema}.${fileset}`
+ const prevFilesetIdentityRef = useRef(filesetIdentity)
+
+ // Track which filesetIdentity the storageLocations prop belongs to.
+ // We record the identity when storageLocations reference changes,
+ // so we can detect when storageLocations is stale (from a previous fileset).
+ const prevStorageLocationsRef = useRef(storageLocations)
+ const storageLocationsIdentityRef = useRef(filesetIdentity)
+ if (prevStorageLocationsRef.current !== storageLocations) {
+ prevStorageLocationsRef.current = storageLocations
+
+ // storageLocations reference changed — record which fileset it belongs to
+ if (storageLocations && Object.keys(storageLocations).length > 0) {
+ storageLocationsIdentityRef.current = filesetIdentity
+ }
+ }
Review Comment:
storageLocationsIdentityRef is only updated when storageLocations is
non-empty. If a fileset legitimately has 0 storage locations ({}),
storageLocationsIsStale can remain true indefinitely and the component will
spin forever. Record the new filesetIdentity whenever the storageLocations prop
reference changes, even if it’s empty.
##########
web-v2/web/src/app/catalogs/rightContent/entitiesContent/ListFiles.js:
##########
@@ -41,11 +41,59 @@ const ListFiles = ({ metalake, catalog, schema, fileset,
storageLocations, defau
const store = useAppSelector(state => state.metalakes)
const dispatch = useAppDispatch()
+ // Track the current fileset identity to detect switches.
+ // Reset navigation state immediately during render (before useEffects run)
+ // so that no stale state can trigger API calls with wrong parameters.
+ const filesetIdentity = `${catalog}.${schema}.${fileset}`
+ const prevFilesetIdentityRef = useRef(filesetIdentity)
+
+ // Track which filesetIdentity the storageLocations prop belongs to.
+ // We record the identity when storageLocations reference changes,
+ // so we can detect when storageLocations is stale (from a previous fileset).
+ const prevStorageLocationsRef = useRef(storageLocations)
+ const storageLocationsIdentityRef = useRef(filesetIdentity)
+ if (prevStorageLocationsRef.current !== storageLocations) {
+ prevStorageLocationsRef.current = storageLocations
+
+ // storageLocations reference changed — record which fileset it belongs to
+ if (storageLocations && Object.keys(storageLocations).length > 0) {
+ storageLocationsIdentityRef.current = filesetIdentity
+ }
+ }
+
+ const filesetSwitched = prevFilesetIdentityRef.current !== filesetIdentity
+ if (filesetSwitched) {
+ prevFilesetIdentityRef.current = filesetIdentity
+ setSubPath('')
+ setPathSegments([])
+ setCurrentLocation(undefined)
Review Comment:
Calling setState during render (inside the filesetSwitched branch) is unsafe
under React concurrent rendering and can lead to hard-to-debug behavior. A
safer approach is to force a remount on fileset identity changes (e.g., add a
changing key on the Files tab wrapper or ListFiles element in
FilesetDetailsPage) and then remove this render-time state reset entirely.
--
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]
