fix: display proper status for empty folders in AI tagging #669
Conversation
|
|
WalkthroughAdds per-folder image counts and tagging timestamps across backend and frontend, updates API schema, refactors Settings UI to show per-folder status (loading/empty/progress) and progress estimates, and extends sync service routes/schemas with timing and richer error responses. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 2
🧹 Nitpick comments (8)
sync-microservice/app/routes/folders.py (3)
50-50: Useloggerinstead ofThe codebase uses a configured logger (as seen in
sync-microservice/app/database/folders.py). Replace- print(f"Tagging status API: {len(folder_info_list)} folders in {round((time.time() - start_time)*1000)}ms") + logger.info(f"Tagging status API: {len(folder_info_list)} folders in {round((time.time() - start_time)*1000)}ms")You'll also need to import and initialize the logger:
from app.logging.setup_logging import get_sync_logger logger = get_sync_logger(__name__)
54-61: Unused exception variablee— consider logging the error details.The captured exception
eis not used, which loses diagnostic information. Log the actual error for debugging while returning a user-friendly message.except sqlite3.Error as e: - + logger.error(f"Database error in tagging status: {e}") return FolderTaggingStatusErrorResponse( status="error", message="Database connection issue - please try again", data=[], total_folders=0 )
62-69: Log the exception details for the generic error handler as well.The exception variable
eis unused here too. Logging helps with debugging unexpected errors.except Exception as e: - + logger.error(f"Unexpected error in tagging status: {e}") return FolderTaggingStatusErrorResponse( status="error", message="Service temporarily unavailable", data=[], total_folders=0 )sync-microservice/app/database/folders.py (1)
110-118: Fix inconsistent indentation.The indentation for
total_imagesandtagged_imagesis inconsistent with the other fields.folder_info_list.append( FolderTaggingInfo( folder_id=folder_id, folder_path=folder_path, - total_images=total_images, - tagged_images=tagged_images, + total_images=total_images, + tagged_images=tagged_images, tagging_percentage=round(tagging_percentage, 2), ) )frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (4)
35-51: Hardcoded 2 seconds per image may be inaccurate.The
estimatedSeconds = remaining * 2assumes a fixed 2 seconds per image. Actual tagging time varies by image complexity, model size, and hardware. Consider:
- Making this configurable
- Calculating based on actual elapsed time and progress
- Documenting this as a rough estimate
For now, this is acceptable as a simple heuristic, but consider extracting the magic number to a named constant.
+const ESTIMATED_SECONDS_PER_IMAGE = 2; + const calculateEstimatedTime = (status: FolderTaggingInfo) => { if (!status || status.total_images === 0 || status.tagging_percentage >= 100) return ''; const processed = status.tagged_images; const total = status.total_images; const remaining = total - processed; - const estimatedSeconds = Math.round(remaining * 2); + const estimatedSeconds = Math.round(remaining * ESTIMATED_SECONDS_PER_IMAGE);
54-70: Extract magic numbers to named constants for clarity.The
3000(3 seconds) grace period is a magic number. Extract it to improve readability and maintainability.+const LOADING_GRACE_PERIOD_MS = 3000; + 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) { + if (status.total_images === 0 && timeSinceUpdate < LOADING_GRACE_PERIOD_MS) { return true; } return false; };
147-181: Inconsistent indentation in JSX block.The JSX block starting at line 147 has inconsistent indentation compared to the rest of the component. Consider aligning it with the surrounding code for better readability.
164-168: Extract threshold constant for ETA visibility.The
50images threshold for showing ETA is a magic number.+const MIN_IMAGES_FOR_ETA_DISPLAY = 50; + // In the JSX: -{status.total_images > 50 && status.tagging_percentage < 100 && ( +{status.total_images > MIN_IMAGES_FOR_ETA_DISPLAY && status.tagging_percentage < 100 && (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/backend/backend_python/openapi.json(2 hunks)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(2 hunks)sync-microservice/app/schemas/folders.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (4)
frontend/src/app/store.ts (1)
RootState(22-22)frontend/src/types/FolderStatus.ts (1)
FolderTaggingInfo(1-7)sync-microservice/app/database/folders.py (1)
FolderTaggingInfo(14-19)sync-microservice/app/schemas/folders.py (1)
FolderTaggingInfo(5-15)
sync-microservice/app/routes/folders.py (3)
sync-microservice/app/database/folders.py (2)
db_get_tagging_progress(75-123)FolderTaggingInfo(14-19)frontend/src/types/FolderStatus.ts (1)
FolderTaggingInfo(1-7)sync-microservice/app/schemas/folders.py (3)
FolderTaggingInfo(5-15)FolderTaggingStatusSuccessResponse(18-23)FolderTaggingStatusErrorResponse(26-36)
🔇 Additional comments (10)
sync-microservice/app/routes/folders.py (1)
31-40: LGTM!The new
total_imagesandtagged_imagesfields are correctly passed from the database layer to the API response schema.docs/backend/backend_python/openapi.json (2)
1119-1123: Verify OpenAPI spec was auto-generated.These schema changes (simplified
$refforinput_type, removedtitle) appear to be from OpenAPI spec regeneration rather than manual edits. Confirm this file is auto-generated and not manually maintained to avoid merge conflicts.
2202-2203:additionalProperties: trueallows arbitrary metadata fields.This change permits any additional properties in the
metadataobject forImageInCluster. This provides flexibility but reduces type safety. Verify this is intentional.frontend/src/types/FolderStatus.ts (1)
1-7: LGTM!The
FolderTaggingInfointerface correctly mirrors the backend schema with the newtotal_imagesandtagged_imagesfields, maintaining type consistency across the stack.sync-microservice/app/database/folders.py (1)
14-19: LGTM!The
FolderTaggingInfoNamedTuple now correctly includestotal_imagesandtagged_imagesfields, enabling the API layer to pass these values through to the frontend.sync-microservice/app/schemas/folders.py (1)
5-15: LGTM!The schema correctly adds
total_imagesandtagged_imageswith appropriatege=0validation constraints, ensuring non-negative values from the API.frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (1)
80-186: Overall implementation achieves PR objectives well.The refactored UI correctly handles three distinct states: loading (with spinner), empty folder (with alert icon), and progress display (with counts and ETA). The 3-second grace period effectively prevents premature "empty folder" display during initial load.
frontend/src/features/folderSlice.ts (3)
8-8: LGTM: Clean state extension for timestamp tracking.The
folderStatusTimestampsfield correctly tracks per-folder status change times, and the initialization is appropriate.Also applies to: 15-15
96-100: LGTM: Complete cleanup of tagging-related state.The
clearTaggingStatusreducer correctly resets all tagging-related state, including the newfolderStatusTimestampsfield.
83-87: I attempted to verify the type definitions but encountered a repository access issue. Let me provide the rewritten review comment based on the information available:
Verify that
total_imagesandtagged_imagesare required (non-optional) fields inFolderTaggingInfo.The change detection logic compares these fields without null/undefined checks. If they are optional fields, the comparison could behave unexpectedly (e.g.,
undefined !== 0evaluates totrue, causing unnecessary timestamp updates). Confirm the type definition infrontend/src/types/FolderStatus.tsor whereverFolderTaggingInfois defined to ensuretotal_imagesandtagged_imagesare required properties.
- Show 'No images found' message for empty folders instead of 0% progress - Add image counts and estimated time for better UX - Implement smart loading states with 3-second grace period - Enhanced progress display with animations
Yogesh-Shivaji365
force-pushed
the
fix/empty-folder-ai-tagging-status
branch
from
22ad8ce to
9083138
Compare
|
is my PR ready for review? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/src/features/folderSlice.ts (1)
77-103: Tagging status update & timestamp cleanup logic looks solidThe
setTaggingStatusreducer correctly:
- Rebuilds
taggingStatusfrom the payload.- Uses a single
now = Date.now()for all updates to keep timestamps comparable.- Only updates
folderStatusTimestampswhentotal_imagesortagged_imagesactually change.- Cleans up timestamps for folders no longer present in the payload, preventing stale entries.
- Updates
lastUpdatedAtin sync with these changes.
clearTaggingStatusalso resets bothtaggingStatusandfolderStatusTimestamps, which keeps the slice state coherent.If you ever want to be extra defensive, you could iterate
Object.keys(state.folderStatusTimestamps)instead offor ... in, but with a plain object from Redux this is more of a style preference than a necessity.Also applies to: 105-110
sync-microservice/app/routes/folders.py (2)
3-4: Success-path timing and payload shaping are correct; prefer structured logging toThe success path looks good:
start_time = time.time()and the elapsed time calculation are correct in milliseconds.FolderTaggingInfonow includestotal_imagesandtagged_images, which aligns with the frontend’s needs.- The unified
responseobject is clean and explicit.Instead of
print(...), consider using your FastAPI app/logger (e.g.,logger.info(...)) so logs are captured by your logging stack and can be filtered/aggregated in production.Also applies to: 27-28, 31-40, 43-52
54-61: Error handling is user-friendly; consider logging exceptions before returningBoth the
sqlite3.Errorand genericExceptionhandlers return sensible, non-leaky error messages with an emptydatalist andtotal_folders=0, which is good for clients.Right now, the caught exceptions (
e) are unused. It would be helpful to log them (e.g.,logger.exception("Failed to fetch folder tagging status")) so operational issues can be diagnosed without exposing details to users.Also applies to: 62-69
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (2)
30-32: Helper logic for ETA and loading state is aligned with requirements
calculateEstimatedTimecorrectly skips ETA when there are no images or the folder is complete, and produces a simple seconds/minutes estimate based on remaining images.isStatusLoadingimplements the intended behavior:
- Returns loading when AI tagging is enabled but no status has arrived yet.
- Applies a 3-second grace period when
total_images === 0before declaring the folder “empty.”- Treats disabled AI tagging as not loading.
To make this a bit more robust against unexpected backend values, you could defensively clamp
remainingto a minimum of 0 before computingestimatedSeconds, e.g.:- const remaining = total - processed; + const remaining = Math.max(total - processed, 0);This avoids negative ETAs if
tagged_imagesever exceedstotal_imagesdue to upstream issues.Also applies to: 35-51, 54-70
82-90: Folder row rendering cleanly separates loading, empty, and progress statesThe per-folder rendering looks good and matches the described UX:
- Uses
key={folder.folder_id}, which is the right stable key.- Computes
status,loading,hasImages,isEmpty, andisCompleteper folder, and only shows the status block whenfolder.AI_Taggingis enabled.- Conditional block correctly prioritizes:
- Loading spinner (“Loading status...”) when
loadingis true.- Empty-state message when
isEmptyis true.- Progress details when
hasImagesis true, including:
- Percentage with a check icon and green styling when complete.
- Tagged/total image counts.
- ETA chip for large folders using
calculateEstimatedTime.- A progress bar that visually distinguishes complete vs in-progress states.
Two very small cleanups you might consider later:
- Drop the unused
indexparameter infolders.map((folder, index) => ...)since the key now usesfolder.folder_id.- If TypeScript ever complains about
statuspossibly being undefined inside thehasImagesbranch, an earlyif (!status) return null;guard before that branch would make the type relationship explicit.Functionally, the status UI and per-folder state handling look correct.
Also applies to: 92-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/backend/backend_python/openapi.json(2 hunks)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(2 hunks)sync-microservice/app/schemas/folders.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/types/FolderStatus.ts
- sync-microservice/app/database/folders.py
- sync-microservice/app/schemas/folders.py
🔇 Additional comments (3)
docs/backend/backend_python/openapi.json (2)
1119-1125: Redundant parameter descriptions in input_type schema.The
input_typeparameter now has duplicate descriptions: one at line 1121 (schema level) and one at line 1124 (parameter level). In OpenAPI 3.1.0, this is valid but creates redundancy. Consider removing the description from the schema to keep it solely at the parameter level, or remove the duplicate at line 1124.Additionally, verify that this manual change to the OpenAPI spec is intentional. If this file is auto-generated from your FastAPI backend code, manual edits may be overwritten on the next generation. Consider regenerating the spec from the backend source to ensure consistency.
2199-2210: Explicit additionalProperties allowance in ImageInCluster metadata.Line 2202 now explicitly allows additional properties on the metadata object (
"additionalProperties": true). This aligns with the PR's goal of flexible metadata handling but loosens the schema validation. Ensure the backend code supports this flexibility and that consumers (frontend, integrations) are aware that metadata may contain unexpected fields.Verify that the backend endpoints returning
ImageInClusteractually populate dynamic/flexible metadata fields and that this schema change reflects the implemented behavior.frontend/src/features/folderSlice.ts (1)
5-16: State shape for per-folder timestamps is consistentAdding
folderStatusTimestampstoFolderStateand initializing it ininitialStateis correct and keeps the timestamp map co-located withtaggingStatus. No issues here.
|
@rahulharpal1603 this PR is ready for your review. |
|
what i do now? |
|
@Yogesh-Shivaji365 |
2 participants
Problem Summary
When AI tagging is enabled on empty folders, the progress bar shows 0% indefinitely instead of displaying a meaningful message that the folder is empty.
Fixes : #592
Solution Implemented
Backend Changes:
Frontend Changes:
Key Benefits
Testing Performed
Screenshots and Video
Images
Video
Defore.video.mp4
After.video.mp4
Summary by CodeRabbit
New Features
API / Backend
✏️ Tip: You can customize this high-level summary in your review settings.