fix: display proper status for empty folders in AI tagging #604
Conversation
WalkthroughAdds per-folder timestamp tracking and image-count fields for AI tagging. Backend returns Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Folder Card UI
participant Slice as Folder Slice
participant API as Backend API
participant Selector as Status Selector
UI->>API: Toggle AI Tagging / request status
API-->>Slice: FolderTaggingInfo { folder_id, total_images, tagged_images, tagging_percentage }
Slice->>Slice: now = Date.now()
Slice->>Slice: if status changed then folderStatusTimestamps[id] = now
Slice-->>Selector: updated state + timestamps
loop Polling / UI refresh
UI->>Selector: isStatusLoading(folderId, hasAITagging)
Selector->>Selector: compare folderStatusTimestamps[id] vs now
alt loading window active
Selector-->>UI: loading = true
UI->>UI: show Loader + status text
else loading window expired
Selector-->>UI: loading = false
alt total_images == 0
UI->>UI: show "No images found in this folder" (Alert)
else total_images > 0
UI->>UI: show progress bar + percentage / completion icon
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
sync-microservice/app/database/folders.py (1)
115-117: Consider adding validation that tagged_images ≤ total_images.While the SQL query logic makes it unlikely for
tagged_imagesto exceedtotal_images, adding an assertion or validation could prevent potential data integrity issues from propagating to the API response.Example validation:
if tagged_images > total_images: logger.warning( f"Data integrity issue in folder {folder_id}: " f"tagged_images ({tagged_images}) > total_images ({total_images})" ) tagged_images = total_imagessync-microservice/app/schemas/folders.py (1)
10-11: Consider adding cross-field validation for data integrity.While individual field validation ensures non-negative values, adding a validator to ensure
tagged_images <= total_imageswould improve data integrity guarantees at the API boundary.Example validator:
from pydantic import field_validator @field_validator('tagged_images') @classmethod def validate_tagged_not_exceeds_total(cls, v, info): total = info.data.get('total_images') if total is not None and v > total: raise ValueError('tagged_images cannot exceed total_images') return v
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/features/folderSlice.ts(2 hunks)frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx(3 hunks)frontend/src/types/FolderStatus.ts(1 hunks)sync-microservice/app/database/folders.py(2 hunks)sync-microservice/app/routes/folders.py(1 hunks)sync-microservice/app/schemas/folders.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (1)
frontend/src/app/store.ts (1)
RootState(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Backend Tests
🔇 Additional comments (10)
frontend/src/types/FolderStatus.ts (1)
4-6: LGTM! Interface correctly extended with image count fields.The addition of
total_imagesandtagged_imagesfields aligns with the backend schema changes and enables the frontend to distinguish between empty folders and folders with 0% progress.sync-microservice/app/database/folders.py (1)
19-20: LGTM! NamedTuple correctly extended with image count fields.The addition of
total_imagesandtagged_imagesto theFolderTaggingInfoNamedTuple properly extends the data model to support the new frontend requirements.sync-microservice/app/routes/folders.py (1)
36-38: LGTM! Route handler correctly propagates new fields.The API response now includes
total_imagesandtagged_images, enabling the frontend to distinguish between empty folders and folders with active tagging progress.frontend/src/features/folderSlice.ts (3)
8-8: LGTM! Per-folder timestamp tracking properly initialized.The addition of
folderStatusTimestampsenables independent loading state management for each folder, preventing cascading effects when folders are added or deleted.Also applies to: 15-15
77-94: LGTM! Timestamp update logic correctly implements change detection.The implementation captures a single timestamp for the batch and updates per-folder timestamps only when
total_imagesortagged_imageschange. This prevents unnecessary timestamp updates while ensuring the UI can detect when folder status has meaningfully changed.Note: The comparison excludes
tagging_percentagesince it's derived from the count fields—this is appropriate.
97-101: LGTM! Cleanup correctly clears timestamp state.The
clearTaggingStatusreducer properly resets both the tagging status and timestamps, ensuring a clean state.frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (4)
31-33: LGTM! Timestamp selector properly integrated.The
folderStatusTimestampsselector enables per-folder loading state computation in theisStatusLoadinghelper.
35-51: LGTM! Loading state logic correctly implements 3-second grace period.The
isStatusLoadinghelper properly handles all states:
- Returns
falsewhen AI tagging is disabled- Returns
trueduring initial load (no status or timestamp)- Implements a 3-second grace period for empty folders to prevent premature display of "No images found"
Note: The grace period transition from loading to empty relies on subsequent API polls to trigger component re-renders. This is acceptable behavior assuming the tagging status API is polled regularly (e.g., every few seconds). The component will update when
taggingStatuschanges, even if the folder's timestamp remains unchanged.
61-67: LGTM! Per-folder state variables correctly computed.The per-folder state computation properly derives
loading,hasImages,isEmpty, andisCompletefrom the tagging status, enabling conditional rendering of appropriate UI elements.
110-146: LGTM! Conditional UI rendering correctly implements three display states.The implementation properly renders:
- Loading state: Spinner with "Loading status..." for folders during initial load or grace period
- Empty state: Alert icon with "No images found in this folder" for confirmed empty folders
- Progress state: Progress bar with percentage, including green completion indicator and check icon at 100%
The logic correctly handles all edge cases, including folders without status data (shows loading) and completed tagging (shows green checkmark).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/features/folderSlice.ts (1)
77-96: Consider cleaning up stale timestamps for removed folders.The timestamp update logic correctly tracks when each folder's status changes. However,
folderStatusTimestampsis not pruned when folders are removed from the payload (e.g., when AI tagging is disabled). This creates a minor memory leak.Consider adding cleanup logic to remove timestamps for folders no longer in the payload:
setTaggingStatus(state, action: PayloadAction<FolderTaggingInfo[]>) { const map: Record<string, FolderTaggingInfo> = {}; const now = Date.now(); + const activeFolderIds = new Set<string>(); for (const info of action.payload) { map[info.folder_id] = info; + activeFolderIds.add(info.folder_id); const existingStatus = state.taggingStatus[info.folder_id]; if ( !existingStatus || existingStatus.total_images !== info.total_images || existingStatus.tagged_images !== info.tagged_images ) { state.folderStatusTimestamps[info.folder_id] = now; } } + // Clean up timestamps for folders no longer in payload + for (const folderId in state.folderStatusTimestamps) { + if (!activeFolderIds.has(folderId)) { + delete state.folderStatusTimestamps[folderId]; + } + } state.taggingStatus = map; state.lastUpdatedAt = now; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/features/folderSlice.ts(2 hunks)frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (1)
frontend/src/app/store.ts (1)
RootState(22-22)
🔇 Additional comments (5)
frontend/src/features/folderSlice.ts (2)
8-8: LGTM! Per-folder timestamp tracking added.The
folderStatusTimestampsfield enables independent loading states for each folder, which aligns with the PR objective to prevent cascading status flickering when folders are added or deleted.
99-103: LGTM! Cleanup logic correctly resets timestamps.The
clearTaggingStatusreducer properly resets bothtaggingStatusandfolderStatusTimestampstogether, maintaining consistency.frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (3)
2-2: LGTM! New icons imported for enhanced status indicators.
Loader2andAlertCircleare correctly used for loading and empty folder states, improving the visual feedback for users.
31-33: LGTM! Selector correctly retrieves per-folder timestamps.The
folderStatusTimestampsselector enables the component to access per-folder update times for calculating loading states.
113-149: LGTM! Three-state UI correctly implements PR objectives.The conditional rendering implements the three distinct states as specified in the PR objectives:
- Loading: Spinner with "Loading status..." during initial fetch or 3-second grace period for empty folders
- Empty: Alert icon with "No images found in this folder" when folder is confirmed empty
- Progress: Progress bar showing tagging percentage, with green styling when complete
This effectively distinguishes between "0 images" and "0% progress," resolving issue #592.
| const isStatusLoading = (folderId: string, folderHasAITagging: boolean) => { | ||
| if (!folderHasAITagging) return false; | ||
|
|
||
| const status = taggingStatus[folderId]; | ||
| if (!status) return true; | ||
|
|
||
| const timestamp = folderStatusTimestamps[folderId]; | ||
| if (!timestamp) return true; | ||
|
|
||
| const timeSinceUpdate = Date.now() - timestamp; | ||
|
|
||
| if (status.total_images === 0 && timeSinceUpdate < 3000) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
Handle missing timestamp more gracefully to avoid indefinite loading.
If status exists but timestamp is missing (line 42), the function returns true (loading), which would show a loading spinner indefinitely. This edge case could occur if state is restored from persistence without timestamps or in rare race conditions.
Consider treating a missing timestamp as "skip grace period" rather than "still loading":
const isStatusLoading = (folderId: string, folderHasAITagging: boolean) => {
if (!folderHasAITagging) return false;
const status = taggingStatus[folderId];
if (!status) return true;
const timestamp = folderStatusTimestamps[folderId];
- if (!timestamp) return true;
-
- const timeSinceUpdate = Date.now() - timestamp;
+ // If no timestamp, treat as "no grace period needed"
+ const timeSinceUpdate = timestamp ? Date.now() - timestamp : Infinity;
if (status.total_images === 0 && timeSinceUpdate < 3000) {
return true;
}
return false;
};This way, if the timestamp is missing, timeSinceUpdate is Infinity, so the 3-second grace period check is bypassed and the actual status (empty or progress) is shown.
📝 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.
| const isStatusLoading = (folderId: string, folderHasAITagging: boolean) => { | |
| if (!folderHasAITagging) return false; | |
| const status = taggingStatus[folderId]; | |
| if (!status) return true; | |
| const timestamp = folderStatusTimestamps[folderId]; | |
| if (!timestamp) return true; | |
| const timeSinceUpdate = Date.now() - timestamp; | |
| if (status.total_images === 0 && timeSinceUpdate < 3000) { | |
| return true; | |
| } | |
| return false; | |
| }; | |
| const isStatusLoading = (folderId: string, folderHasAITagging: boolean) => { | |
| if (!folderHasAITagging) return false; | |
| const status = taggingStatus[folderId]; | |
| if (!status) return true; | |
| const timestamp = folderStatusTimestamps[folderId]; | |
| // If no timestamp, treat as "no grace period needed" | |
| const timeSinceUpdate = timestamp ? Date.now() - timestamp : Infinity; | |
| if (status.total_images === 0 && timeSinceUpdate < 3000) { | |
| return true; | |
| } | |
| return false; | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx around
lines 35 to 51, the function treats a missing timestamp as "still loading" which
can cause an indefinite spinner; change the logic so the 3-second grace period
is only applied when a valid timestamp exists (e.g., if timestamp is missing,
skip the grace check by treating timeSinceUpdate as Infinity or by bypassing the
<3000 check), so status with no timestamp falls back to showing the actual state
instead of staying in loading.
| /> | ||
| return ( | ||
| <div | ||
| key={index} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use folder.folder_id as key instead of array index.
Using array index as the key prop is a React anti-pattern that can cause rendering issues when folders are reordered, added, or removed. React may incorrectly recycle component instances, leading to stale state or incorrect UI updates.
Apply this diff:
- key={index}
+ key={folder.folder_id}📝 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.
| key={index} | |
| key={folder.folder_id} |
🤖 Prompt for AI Agents
In frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx around
line 73, the component uses the array index as the React key which can cause
incorrect re-renders; replace key={index} with key={folder.folder_id} (or
another stable unique id from the folder object) to provide a stable and unique
key for each item, and if folder.folder_id might be undefined add a safe
fallback (e.g., a generated unique id stored on the folder) to ensure keys
remain stable across renders.
1 participant
Changes
Backend (Sync Microservice)
total_imagesandtagged_imagesfields to tagging status API responseFrontend
folderStatusTimestamps)Key Improvements
fix_issue_592.mp4
Summary by CodeRabbit