fix: display proper status for empty folders in AI tagging#604
fix: display proper status for empty folders in AI tagging#604SiddharthJiyani wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR extends the folder AI tagging feature to distinguish between empty folders and those actively processing. Backend now tracks total and tagged image counts, enabling the frontend to render loading state, empty-folder messages, or progress conditionally based on per-folder timestamps and image count data. ChangesFolder Tagging Status Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (2)
73-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
key={index}— unresolved from previous review.Using the array index as the React key causes incorrect DOM recycling when folders are reordered, added, or removed.
- key={index} + key={folder.folder_id}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx` at line 73, In FolderManagementCard where the list item uses key={index}, replace the index key with a stable unique identifier from the folder object (e.g., folder.id or folder.path/name) used in the map render so React can track items correctly when reordering/adding/removing; update the mapped element where key={index} appears to use folder.id (or folder.path/name) and if some folders lack an id, derive or persist a stable id (generate once and store on the folder record or use a deterministic key) rather than falling back to the array index.
41-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
!timestampcauses indefinite loading spinner — unresolved from previous review.When
statusis present buttimestampis absent (e.g., persisted Redux state missing the new field),isStatusLoadingalways returnstrue, pinning the folder in the loading state forever.🛠️ Proposed fix
const timestamp = folderStatusTimestamps[folderId]; - if (!timestamp) return true; - - const timeSinceUpdate = Date.now() - timestamp; + // If timestamp is missing, skip the grace period and show actual state + const timeSinceUpdate = timestamp ? Date.now() - timestamp : Infinity; if (status.total_images === 0 && timeSinceUpdate < 3000) {
Infinitybypasses the< 3000check, so a missing timestamp falls back to displaying the real state rather than spinning indefinitely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx` around lines 41 - 42, The check in isStatusLoading uses folderStatusTimestamps[folderId] and currently does `if (!timestamp) return true`, which treats a missing timestamp as "loading" forever; instead, when timestamp is missing (undefined or null) return Infinity (or a very large number) so that the subsequent `< 3000` staleness check evaluates false and the real status displays. Update the logic in the function that reads folderStatusTimestamps (referencing folderStatusTimestamps and folderId, e.g., inside isStatusLoading) to explicitly detect undefined/null and return Infinity rather than returning true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx`:
- Line 73: In FolderManagementCard where the list item uses key={index}, replace
the index key with a stable unique identifier from the folder object (e.g.,
folder.id or folder.path/name) used in the map render so React can track items
correctly when reordering/adding/removing; update the mapped element where
key={index} appears to use folder.id (or folder.path/name) and if some folders
lack an id, derive or persist a stable id (generate once and store on the folder
record or use a deterministic key) rather than falling back to the array index.
- Around line 41-42: The check in isStatusLoading uses
folderStatusTimestamps[folderId] and currently does `if (!timestamp) return
true`, which treats a missing timestamp as "loading" forever; instead, when
timestamp is missing (undefined or null) return Infinity (or a very large
number) so that the subsequent `< 3000` staleness check evaluates false and the
real status displays. Update the logic in the function that reads
folderStatusTimestamps (referencing folderStatusTimestamps and folderId, e.g.,
inside isStatusLoading) to explicitly detect undefined/null and return Infinity
rather than returning true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4ca82ce-e216-4f34-9cb1-f233b2fcba0c
📒 Files selected for processing (6)
frontend/src/features/folderSlice.tsfrontend/src/pages/SettingsPage/components/FolderManagementCard.tsxfrontend/src/types/FolderStatus.tssync-microservice/app/database/folders.pysync-microservice/app/routes/folders.pysync-microservice/app/schemas/folders.py
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect AI tagging status display for empty folders by extending the backend tagging-status API to include explicit image counts and updating the frontend UI logic to differentiate “loading”, “empty”, and “in-progress” states per folder.
Changes:
- Backend: include
total_imagesandtagged_imagesin folder tagging status data from DB → schema → route response. - Frontend: track per-folder status timestamps in Redux to drive independent loading/empty/progress rendering with a 3s grace period for 0-image folders.
- Frontend UI: add explicit “Loading status…” and “No images found in this folder” states instead of showing a stuck 0% progress bar.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sync-microservice/app/schemas/folders.py | Adds total_images/tagged_images to the tagging status response schema. |
| sync-microservice/app/routes/folders.py | Populates the new image-count fields in the /folders/status response. |
| sync-microservice/app/database/folders.py | Returns total/tagged image counts alongside computed tagging percentage. |
| frontend/src/types/FolderStatus.ts | Extends the frontend type to include total_images/tagged_images. |
| frontend/src/features/folderSlice.ts | Stores per-folder “counts changed” timestamps to support stable UI state transitions. |
| frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx | Renders per-folder loading/empty/progress states using timestamps and new API fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const isStatusLoading = (folderId: string, folderHasAITagging: boolean) => { | ||
| if (!folderHasAITagging) return false; | ||
|
|
||
| const status = taggingStatus[folderId]; | ||
| if (!status) return true; | ||
|
|
||
| const timestamp = folderStatusTimestamps[folderId]; | ||
| const timeSinceUpdate = timestamp ? Date.now() - timestamp : Infinity; | ||
|
|
||
| if (status.total_images === 0 && timeSinceUpdate < 3000) { | ||
| return true; |
| const timeSinceUpdate = timestamp ? Date.now() - timestamp : Infinity; | ||
|
|
||
| if (status.total_images === 0 && timeSinceUpdate < 3000) { | ||
| return true; |
| setTaggingStatus(state, action: PayloadAction<FolderTaggingInfo[]>) { | ||
| const map: Record<string, FolderTaggingInfo> = {}; | ||
| const now = Date.now(); | ||
|
|
||
| for (const info of action.payload) { | ||
| map[info.folder_id] = info; | ||
|
|
||
| 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; | ||
| } | ||
| } | ||
|
|
||
| state.taggingStatus = map; | ||
| state.lastUpdatedAt = Date.now(); | ||
| state.lastUpdatedAt = now; |
| {folder.AI_Tagging && ( | ||
| <div className="mt-3"> | ||
| {loading ? ( | ||
| <div className="text-muted-foreground flex items-center gap-2 text-xs"> | ||
| <Loader2 className="h-3 w-3 animate-spin" /> | ||
| <span>Loading status...</span> | ||
| </div> | ||
| ) : isEmpty ? ( | ||
| <div className="text-muted-foreground flex items-center gap-2 text-xs"> | ||
| <AlertCircle className="h-3 w-3" /> | ||
| <span>No images found in this folder</span> | ||
| </div> | ||
| ) : hasImages ? ( | ||
| <> | ||
| <div className="text-muted-foreground mb-1 flex items-center justify-between text-xs"> | ||
| <span>AI Tagging Progress</span> | ||
| <span | ||
| className={ | ||
| isComplete | ||
| ? 'flex items-center gap-1 text-green-500' | ||
| : 'text-muted-foreground' | ||
| } | ||
| > | ||
| {isComplete && <Check className="h-3 w-3" />} | ||
| {Math.round(status.tagging_percentage)}% | ||
| </span> | ||
| </div> | ||
| <Progress | ||
| value={status.tagging_percentage} | ||
| indicatorClassName={ | ||
| isComplete ? 'bg-green-500' : 'bg-blue-500' | ||
| } | ||
| /> | ||
| </> | ||
| ) : null} |
rohan-pandeyy
left a comment
rohan-pandeyy
left a comment
There was a problem hiding this comment.
Hi @SiddharthJiyani,
Thanks for the PR!
Currently, there's a "randomness" caused by a race condition between how long the user's backend takes to scan a folder on the disk, and the hardcoded 3-second timeout.
If the user's folder has only a few images or the disk is fast, the backend might finish the scan in 1.5 seconds (comfortably inside the 3-second grace period👍) but if the folder is large and takes 4-6 seconds to scan, we see the "No images found" flicker.
We should avoid time-guessing. Modify the backend to return an explicit is_scanning: boolean flag for each folder, or return status.total_images = null instead of 0 while the initial disk scan is still in progress. The frontend can then simply say: const loading = status.is_scanning;
Before you start, pull the latest changes into your SiddharthJiyani:592-fix-empty-folder-ai-tagging-status branch. Then continue with the backend changes, otherwise we’ll run into merge conflicts with the master branch.
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
Changes
Backend (Sync Microservice)
total_imagesandtagged_imagesfields to tagging status API responseFrontend
folderStatusTimestamps)Key Improvements
fix_issue_592.mp4
Summary by CodeRabbit