-
Notifications
You must be signed in to change notification settings - Fork 597
Implement live progress tracking with 1s polling, animated progress b… #574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ars, and completion indicators for AI tagging status in folder management.
WalkthroughThis PR adds AI tagging progress tracking for folders. It introduces a new API endpoint to fetch tagging status, defines TypeScript types for the response structure, adds Redux state management for storing tagging progress per folder, implements polling logic in a custom hook, and displays progress indicators in the folder management UI. Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as useFolderOperations
participant Query as useQuery<br/>(Polling)
participant API as getFoldersTaggingStatus
participant Store as Redux Store
participant UI as FolderManagementCard
loop Every 1 second
Query->>API: GET /folders/status
API-->>Query: FolderTaggingStatusResponse
Query->>Store: dispatch setTaggingStatus(data)
Store-->>Store: Update taggingStatus record
Note over Store: taggingStatus keyed by<br/>folder_id
end
Store->>UI: useSelector(taggingStatus)
UI->>UI: Render Progress bar<br/>per AI_TAGGING folder
UI->>UI: Show percentage & check<br/>icon when progress=100%
Note over UI: Green color<br/>at 100%
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple layers (API, types, Redux, hooks, UI components) with new polling logic and state management, but the individual modifications are straightforward and focused on a single feature. The logic density is moderate, and the changes follow established patterns in the codebase. Possibly related PRs
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (1)
53-84: Extract repeated expression to improve readability.The expression
taggingStatus[folder.folder_id]?.tagging_percentage ?? 0is repeated 6 times. Extract it to a variable for better maintainability.Apply this diff:
</div> {folder.AI_Tagging && ( <div className="mt-3"> + {(() => { + const percentage = taggingStatus[folder.folder_id]?.tagging_percentage ?? 0; + const isComplete = percentage >= 100; + return ( + <> <div className="text-muted-foreground mb-1 flex items-center justify-between text-xs"> <span>AI Tagging Progress</span> <span className={ - (taggingStatus[folder.folder_id]?.tagging_percentage ?? 0) >= 100 - ? 'text-green-500 flex items-center gap-1' + isComplete ? 'text-green-500 flex items-center gap-1' : 'text-muted-foreground' } > - {(taggingStatus[folder.folder_id]?.tagging_percentage ?? 0) >= 100 && ( + {isComplete && ( <Check className="h-3 w-3" /> )} - {Math.round( - taggingStatus[folder.folder_id]?.tagging_percentage ?? 0, - )} - % + {Math.round(percentage)}% </span> </div> <Progress - value={ - taggingStatus[folder.folder_id]?.tagging_percentage ?? 0 - } + value={percentage} indicatorClassName={ - (taggingStatus[folder.folder_id]?.tagging_percentage ?? 0) >= 100 - ? 'bg-green-500' + isComplete ? 'bg-green-500' : 'bg-blue-500' } /> + </> + ); + })()} </div> )}frontend/src/api/api-functions/folders.ts (1)
74-84: Improve type safety in the response mapping.The
as anycast on line 80 bypasses type checking. While this works at runtime, it hides a type mismatch betweenFolderTaggingInfo[]and theAPIResponse.datastructure.Consider one of these approaches:
Option 1: Update APIResponse to support array data
export interface APIResponse<T = any> { data?: T; success: boolean; error?: string; message?: string; }Then update the function:
-export const getFoldersTaggingStatus = async (): Promise<APIResponse> => { +export const getFoldersTaggingStatus = async (): Promise<APIResponse<FolderTaggingInfo[]>> => { const response = await syncApiClient.get<FolderTaggingStatusResponse>( foldersEndpoints.getTaggingStatus, ); const res = response.data; return { - data: res.data as any, + data: res.data, success: res.status === 'success', message: res.message, }; };Option 2: Keep wrapping but be explicit
return { - data: res.data as any, + data: res.data as FolderTaggingInfo[] as any, success: res.status === 'success', message: res.message, };frontend/src/features/taggingStatusSlice.ts (1)
17-27: Consider using a more idiomatic map construction.The manual loop works correctly but could be simplified.
Apply this diff for a more concise approach:
setTaggingStatus( state, action: PayloadAction<FolderTaggingInfo[]>, ) { - const map: Record<string, FolderTaggingInfo> = {}; - for (const info of action.payload) { - map[info.folder_id] = info; - } - state.byFolderId = map; + state.byFolderId = Object.fromEntries( + action.payload.map(info => [info.folder_id, info]) + ); state.lastUpdatedAt = Date.now(); }Note: This implementation replaces the entire map on each update, meaning folders absent from the new payload will be removed from state. If this is not the intended behavior, consider merging instead:
state.byFolderId = { ...state.byFolderId, ...Object.fromEntries(action.payload.map(info => [info.folder_id, info])) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/api/api-functions/folders.ts(2 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/app/store.ts(2 hunks)frontend/src/components/ui/progress.tsx(1 hunks)frontend/src/features/taggingStatusSlice.ts(1 hunks)frontend/src/hooks/useFolderOperations.tsx(3 hunks)frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx(4 hunks)frontend/src/types/FolderStatus.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/src/hooks/useFolderOperations.tsx (3)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/folders.ts (1)
getFoldersTaggingStatus(74-84)frontend/src/features/taggingStatusSlice.ts (1)
setTaggingStatus(17-27)
frontend/src/components/ui/progress.tsx (1)
frontend/src/lib/utils.ts (1)
cn(5-7)
frontend/src/features/taggingStatusSlice.ts (1)
frontend/src/types/FolderStatus.ts (1)
FolderTaggingInfo(1-5)
frontend/src/api/api-functions/folders.ts (4)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/axiosConfig.ts (1)
syncApiClient(13-20)frontend/src/types/FolderStatus.ts (1)
FolderTaggingStatusResponse(7-12)frontend/src/api/apiEndpoints.ts (1)
foldersEndpoints(12-20)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (1)
frontend/src/app/store.ts (1)
RootState(24-24)
🔇 Additional comments (5)
frontend/src/api/apiEndpoints.ts (1)
19-19: LGTM!The new endpoint follows the existing naming conventions and structure.
frontend/src/types/FolderStatus.ts (1)
1-12: LGTM!The type definitions are clear and well-documented. The inline comment for
tagging_percentagerange (0-100) is helpful.frontend/src/app/store.ts (1)
9-9: LGTM!The Redux store configuration correctly integrates the new tagging status slice.
Also applies to: 20-20
frontend/src/components/ui/progress.tsx (2)
6-8: LGTM! The custom indicator styling is well-implemented.The
indicatorClassNameprop and enhanced animations provide good flexibility for consumers while maintaining the component's core functionality.Also applies to: 27-30
20-20: Verify Progress container background change
The default background forProgress.Rootwas changed frombg-primary/20tobg-white, affecting usages in:
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsxfrontend/src/components/Updater/UpdateDialog.tsx
Ensure the new white background renders correctly with surrounding UI.
…nd validating API responses before Redux dispatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/hooks/useFolderOperations.tsx (2)
68-76: Optional: Improve type safety.The
as anycast followed byArray.isArrayvalidation is safe but not ideal. Consider typing the data more precisely based on the API response structure.Example:
useEffect(() => { if (taggingStatusQuery.data?.success) { - const raw = taggingStatusQuery.data.data as any; + const raw = taggingStatusQuery.data.data as unknown; if (Array.isArray(raw)) { dispatch(setTaggingStatus(raw)); } } }, [taggingStatusQuery.data, dispatch]);Or, if
FolderTaggingInfo[]is the expected type, validate and cast accordingly:+import { FolderTaggingInfo } from '@/types/FolderStatus'; + useEffect(() => { if (taggingStatusQuery.data?.success) { - const raw = taggingStatusQuery.data.data as any; - if (Array.isArray(raw)) { + const raw = taggingStatusQuery.data.data; + if (Array.isArray(raw) && raw.every((item): item is FolderTaggingInfo => + typeof item === 'object' && 'folder_id' in item + )) { dispatch(setTaggingStatus(raw)); } } }, [taggingStatusQuery.data, dispatch]);
78-86: Simplify redundant logging.Both
console.error(line 80) andconsole.warn(line 83) are logging the same failure. Consider consolidating to a single log statement.Apply this diff to streamline logging:
useEffect(() => { if (taggingStatusQuery.isError) { + const errorMessage = taggingStatusQuery.errorMessage || 'Unknown error'; - console.error('Failed to fetch tagging status:', taggingStatusQuery.error); - - const errorMessage = taggingStatusQuery.errorMessage || 'Unknown error'; - console.warn(`Tagging status query failed: ${errorMessage}`); - + console.error(`Failed to fetch tagging status: ${errorMessage}`, taggingStatusQuery.error); } }, [taggingStatusQuery.isError, taggingStatusQuery.error, taggingStatusQuery.errorMessage]);Note: The current error handling is passive (no user notification). This may be intentional for background polling, but be aware that users won't be alerted if status updates become stale due to persistent API failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/hooks/useFolderOperations.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/hooks/useFolderOperations.tsx (3)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/folders.ts (1)
getFoldersTaggingStatus(74-84)frontend/src/features/taggingStatusSlice.ts (1)
setTaggingStatus(17-27)
🔇 Additional comments (1)
frontend/src/hooks/useFolderOperations.tsx (1)
14-15: LGTM!The new imports are necessary for polling tagging status and updating Redux state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/hooks/useFolderOperations.tsx (1)
31-41: Stop polling once all active folders reach 100% and avoid background polling if not needed.Current
enabled: folders.some((f) => f.AI_Tagging)keeps polling even after completion and with the tab unfocused, impacting battery/CPU. Gate by actual in‑progress status from Redux and consider disabling background polling.+import { RootState } from '@/app/store'; @@ - const taggingStatusQuery = usePictoQuery({ + const taggingStatusMap = useSelector( + (s: RootState) => s.taggingStatus.byFolderId, + ); + const hasActiveTagging = folders.some((f) => { + if (!f.AI_Tagging) return false; + const st = taggingStatusMap[f.folder_id]; + return !st || (st.tagging_percentage ?? 0) < 100; + }); + + const taggingStatusQuery = usePictoQuery({ queryKey: ['folders', 'tagging-status'], queryFn: getFoldersTaggingStatus, staleTime: 1000, refetchInterval: 1000, - refetchIntervalInBackground: true, - enabled: folders.some((f) => f.AI_Tagging), + refetchIntervalInBackground: false, + enabled: hasActiveTagging,Optional: when
hasActiveTaggingflips to false, dispatchclearTaggingStatus()to drop stale state.
🧹 Nitpick comments (5)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (2)
41-43: Use a stable key instead of the array index.Index keys can cause unnecessary re-renders and subtle UI bugs when the list changes. Prefer the folder’s id.
- <div - key={index} + <div + key={folder.folder_id}
53-89: Deduplicate lookups and clamp progress to [0,100] to avoid UI glitches.You compute tagging percentage multiple times and don’t clamp bounds. Cache once and clamp; simplifies JSX and prevents overflow if the API briefly reports >100.
- {folder.AI_Tagging && ( - <div className="mt-3"> + {folder.AI_Tagging && (() => { + const pct = Math.min( + 100, + Math.max( + 0, + Math.round(taggingStatus[folder.folder_id]?.tagging_percentage ?? 0), + ), + ); + const done = pct >= 100; + return ( + <div className="mt-3"> <div className="text-muted-foreground mb-1 flex items-center justify-between text-xs"> <span>AI Tagging Progress</span> - <span - className={ - (taggingStatus[folder.folder_id] - ?.tagging_percentage ?? 0) >= 100 - ? 'flex items-center gap-1 text-green-500' - : 'text-muted-foreground' - } - > - {(taggingStatus[folder.folder_id] - ?.tagging_percentage ?? 0) >= 100 && ( + <span className={done ? 'flex items-center gap-1 text-green-500' : 'text-muted-foreground'}> + {done && ( <Check className="h-3 w-3" /> )} - {Math.round( - taggingStatus[folder.folder_id] - ?.tagging_percentage ?? 0, - )} + {pct} % </span> </div> <Progress - value={ - taggingStatus[folder.folder_id]?.tagging_percentage ?? - 0 - } - indicatorClassName={ - (taggingStatus[folder.folder_id] - ?.tagging_percentage ?? 0) >= 100 - ? 'bg-green-500' - : 'bg-blue-500' - } + value={pct} + indicatorClassName={done ? 'bg-green-500' : 'bg-blue-500'} /> - </div> - )} + </div> + ); + })()}frontend/src/hooks/useFolderOperations.tsx (1)
78-87: Reduce console noise and surface errors via existing feedback utilities.Console spam every second can be noisy. Consider throttling logs or routing through your toast/feedback system only on state transitions (first failure after success).
If helpful, I can wire this to your existing
useMutationFeedback/toast path on error transitions.frontend/src/components/ui/progress.tsx (2)
20-21: Theming:bg-whitemay clash in dark mode.Prefer a tokened background (e.g.,
bg-mutedorbg-primary/20) to respect themes.- 'relative h-2 w-full overflow-hidden rounded-full bg-white', + 'relative h-2 w-full overflow-hidden rounded-full bg-muted',
27-32: Clamp value and respect reduced motion.Prevent overflow with clamping and add
motion-reducefallbacks for accessibility.-function Progress({ +function Progress({ className, value, indicatorClassName, ...props }: ProgressProps) { - return ( + const clamped = Math.min(100, Math.max(0, value ?? 0)); + return ( @@ - <ProgressPrimitive.Indicator + <ProgressPrimitive.Indicator data-slot="progress-indicator" className={cn( - 'bg-primary h-full w-full flex-1 transition-transform duration-700 ease-in-out will-change-transform', + 'bg-primary h-full w-full flex-1 transition-transform duration-700 ease-in-out will-change-transform motion-reduce:transition-none motion-reduce:transform-none', indicatorClassName, )} - style={{ transform: `translateX(-${100 - (value || 0)}%)` }} + style={{ transform: `translateX(-${100 - clamped}%)` }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/components/ui/progress.tsx(1 hunks)frontend/src/features/taggingStatusSlice.ts(1 hunks)frontend/src/hooks/useFolderOperations.tsx(3 hunks)frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx(3 hunks)frontend/src/types/FolderStatus.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/features/taggingStatusSlice.ts
- frontend/src/types/FolderStatus.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/hooks/useFolderOperations.tsx (3)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/folders.ts (1)
getFoldersTaggingStatus(74-84)frontend/src/features/taggingStatusSlice.ts (1)
setTaggingStatus(17-24)
frontend/src/components/ui/progress.tsx (1)
frontend/src/lib/utils.ts (1)
cn(5-7)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (1)
frontend/src/app/store.ts (1)
RootState(24-24)
🔇 Additional comments (1)
frontend/src/hooks/useFolderOperations.tsx (1)
70-75: Good fix: validating API success before dispatch.The success guard prevents bad data from entering Redux. LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/features/folderSlice.ts (2)
69-72: Clear tagging status when folders are cleared.The
clearFoldersreducer does not cleartaggingStatus, leaving orphaned entries for removed folders. This could cause memory leaks or stale data issues if folders are repeatedly added and removed.Apply this diff to also clear tagging status:
clearFolders(state) { state.folders = []; + state.taggingStatus = {}; + state.lastUpdatedAt = undefined; }
61-67: Clean up tagging status for removed folders.When folders are removed, their tagging status entries remain in the map, causing potential memory leaks and stale data.
Apply this diff to remove orphaned tagging status:
removeFolders(state, action: PayloadAction<string[]>) { const folderIdsToRemove = action.payload; state.folders = state.folders.filter( (folder) => !folderIdsToRemove.includes(folder.folder_id), ); + folderIdsToRemove.forEach((id) => { + delete state.taggingStatus[id]; + }); }
♻️ Duplicate comments (1)
frontend/src/hooks/useFolderOperations.tsx (1)
30-40: Polling continues after tagging completes.The
enabledcondition only checks whether any folder hasAI_Taggingenabled, not whether tagging is actively in progress. Once a folder reaches 100% completion, polling continues indefinitely until AI_Tagging is manually disabled, wasting resources.Consider checking the tagging status from Redux to stop polling when all folders with
AI_Tagging: truehave reached 100% completion. For example:const taggingStatus = useSelector((state: RootState) => state.folders.taggingStatus); const hasActiveTagging = folders.some((f) => { if (!f.AI_Tagging) return false; const status = taggingStatus[f.folder_id]; return !status || status.progress < 100; }); const taggingStatusQuery = usePictoQuery({ // ... other options enabled: hasActiveTagging, });
🧹 Nitpick comments (1)
frontend/src/hooks/useFolderOperations.tsx (1)
77-91: Consider user feedback for persistent errors.Error logging is good, but users receive no feedback when tagging status polling fails repeatedly. While this is background polling and not critical, consider adding a subtle UI indicator or limiting console warnings after the initial failure to reduce noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/features/folderSlice.ts(3 hunks)frontend/src/hooks/useFolderOperations.tsx(3 hunks)frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/features/folderSlice.ts (2)
backend/app/schemas/folders.py (1)
FolderDetails(26-32)frontend/src/types/Folder.ts (1)
FolderDetails(1-8)
frontend/src/hooks/useFolderOperations.tsx (3)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/folders.ts (1)
getFoldersTaggingStatus(74-84)frontend/src/features/folderSlice.ts (1)
setTaggingStatus(75-82)
🔇 Additional comments (4)
frontend/src/hooks/useFolderOperations.tsx (1)
67-75: LGTM! Success validation properly implemented.The effect now correctly validates the API response success flag before dispatching to Redux, ensuring only valid data is stored.
frontend/src/features/folderSlice.ts (3)
3-3: LGTM! State structure is well-typed.The addition of
taggingStatusas a keyed record andlastUpdatedAttimestamp provides efficient lookup and tracking capabilities.Also applies to: 7-8
74-82: LGTM! Reducer correctly transforms array to map.The reducer efficiently converts the array payload to a keyed record for O(1) lookups and updates the timestamp.
84-88: LGTM! Clear operation is correct.The reducer properly resets both the tagging status map and the timestamp.
rahulharpal1603
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @SiddharthJiyani !
This pull request introduces a new feature to display real-time AI tagging progress for folders in the Settings page. It does so by polling the backend for tagging status, storing the results in Redux, and showing a progress bar for each folder with AI tagging enabled. The changes include new API integration, Redux state management, and UI enhancements.
API integration and state management:
getFoldersTaggingStatusinfolders.tsto fetch the AI tagging progress for all folders from the sync microservice, and updatedapiEndpoints.tsto include the new endpoint.taggingStatusSliceto store and manage tagging progress per folder, and integrated it into the main Redux store.useFolderOperationshook to poll the tagging status API every second (only when AI tagging is enabled) and update the Redux store with the latest progress.UI enhancements:
FolderManagementCardcomponent to display a progress bar and percentage for each folder with AI tagging enabled, with blue progress bars and green completion state including a checkmark when tagging is complete.ProgressUI component to support custom indicator colors and smoother animations, making it suitable for displaying tagging progress.Type definitions:
FolderStatus.tsto describe the structure of tagging status API responses and folder tagging information.AI_Tagging_Pictopy.mp4
Summary by CodeRabbit
Release Notes