Skip to content
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions frontend/src/components/FaceCollections.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,16 @@ export function FaceCollections() {
});

useEffect(() => {
if (clustersSuccess && clustersData?.data?.clusters) {
const clusters = (clustersData.data.clusters || []) as Cluster[];
dispatch(setClusters(clusters));
}
}, [clustersData, clustersSuccess, dispatch]);
if (clustersSuccess && clustersData?.data?.clusters) {
const clusters = (clustersData.data.clusters || []) as Cluster[];

const filteredClusters = clusters.filter(c => c.face_count > 0);

dispatch(setClusters(filteredClusters));
console.log(filteredClusters);
}
}, [clustersData, clustersSuccess, dispatch]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Filtering logic is correct; remove console.log and improve variable naming.

The core fix correctly filters out clusters with face_count === 0, which addresses the stated objective of preventing face data persistence after folder removal.

However:

  1. Remove the console.log on line 30 before merging to production. Logging filtered data is helpful during development but should not ship to users.

  2. The local variable clusters on line 25 shadows the Redux state variable from line 16, making the code harder to follow. Consider renaming the local variable to allClusters or fetchedClusters.

  3. The || [] fallback on line 25 is redundant since line 24 already ensures clustersData?.data?.clusters exists via optional chaining.

Apply this diff to address the issues:

  useEffect(() => {
   if (clustersSuccess && clustersData?.data?.clusters) {
-    const clusters = (clustersData.data.clusters || []) as Cluster[];
+    const fetchedClusters = clustersData.data.clusters as Cluster[];

-    const filteredClusters = clusters.filter(c => c.face_count > 0);
+    const filteredClusters = fetchedClusters.filter(c => c.face_count > 0);

     dispatch(setClusters(filteredClusters));
-    console.log(filteredClusters);
   }
 }, [clustersData, clustersSuccess, dispatch]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
if (clustersSuccess && clustersData?.data?.clusters) {
const clusters = (clustersData.data.clusters || []) as Cluster[];
dispatch(setClusters(clusters));
}
}, [clustersData, clustersSuccess, dispatch]);
if (clustersSuccess && clustersData?.data?.clusters) {
const clusters = (clustersData.data.clusters || []) as Cluster[];
const filteredClusters = clusters.filter(c => c.face_count > 0);
dispatch(setClusters(filteredClusters));
console.log(filteredClusters);
}
}, [clustersData, clustersSuccess, dispatch]);
useEffect(() => {
if (clustersSuccess && clustersData?.data?.clusters) {
const fetchedClusters = clustersData.data.clusters as Cluster[];
const filteredClusters = fetchedClusters.filter(c => c.face_count > 0);
dispatch(setClusters(filteredClusters));
}
}, [clustersData, clustersSuccess, dispatch]);
🤖 Prompt for AI Agents
In frontend/src/components/FaceCollections.tsx around lines 23 to 32, remove the
development console.log call, rename the local variable `clusters` to
`fetchedClusters` (or `allClusters`) to avoid shadowing the Redux state
variable, and eliminate the redundant `|| []` fallback since the optional
chaining already guarantees the existence check; then dispatch the filtered
results using the new variable name.



const handlePersonClick = (clusterId: string) => {
navigate(`/person/${clusterId}`);
Expand Down
Loading