feat(folder-ui): add bulk AI tagging controls, smart sorting, and pro… #787
Conversation
|
|
WalkthroughAdds bulk AI-tagging UI and hook methods, introduces a small SQLite migration utility and migration docs, refactors logging interception handling, and makes minor OpenAPI schema adjustments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (11)
frontend/src/hooks/useFolderOperations.tsx (3)
163-173: Add user feedback for bulk mutations.The single-folder mutations use
useMutationFeedbackto show loading/success/error toasts, but bulk mutations lack this feedback. Users won't know when a bulk operation completes or fails.+ // Apply feedback to the bulk enable AI tagging mutation + useMutationFeedback(enableBulkAITaggingMutation, { + showLoading: true, + loadingMessage: 'Enabling AI tagging for selected folders', + successTitle: 'Bulk AI Tagging Enabled', + successMessage: 'AI tagging has been enabled for the selected folders.', + errorTitle: 'Bulk AI Tagging Error', + errorMessage: 'Failed to enable AI tagging for some folders. Please try again.', + }); + + // Apply feedback to the bulk disable AI tagging mutation + useMutationFeedback(disableBulkAITaggingMutation, { + showLoading: true, + loadingMessage: 'Disabling AI tagging for selected folders', + successTitle: 'Bulk AI Tagging Disabled', + successMessage: 'AI tagging has been disabled for the selected folders.', + errorTitle: 'Bulk AI Tagging Error', + errorMessage: 'Failed to disable AI tagging for some folders. Please try again.', + });
175-185: Enforce the 50-folder bulk operation limit.Per the linked issue requirements, bulk operations should be limited to 50 folders. Large libraries could exceed this limit. Consider chunking or warning users.
+const BULK_LIMIT = 50; + const tagAllFolders = () => { const ids = folders.filter((f) => !f.AI_Tagging).map((f) => f.folder_id); - if (ids.length > 0) enableBulkAITaggingMutation.mutate(ids); + if (ids.length > 0) { + // Respect backend limit by chunking + const chunk = ids.slice(0, BULK_LIMIT); + enableBulkAITaggingMutation.mutate(chunk); + if (ids.length > BULK_LIMIT) { + console.warn(`Only first ${BULK_LIMIT} folders will be tagged. ${ids.length - BULK_LIMIT} remaining.`); + } + } }; const tagSelectedFolders = (selectedIds: string[]) => { const pending = selectedIds.filter( (id) => !folders.find((f) => f.folder_id === id)?.AI_Tagging, ); - if (pending.length > 0) enableBulkAITaggingMutation.mutate(pending); + if (pending.length > 0) { + const chunk = pending.slice(0, BULK_LIMIT); + enableBulkAITaggingMutation.mutate(chunk); + if (pending.length > BULK_LIMIT) { + console.warn(`Only first ${BULK_LIMIT} folders will be tagged. ${pending.length - BULK_LIMIT} remaining.`); + } + } };Alternatively, iterate through chunks sequentially or surface a user-facing warning in the UI.
169-173:disableBulkAITaggingMutationis unused.The mutation is defined and its pending state is exposed, but no function calls
disableBulkAITaggingMutation.mutate(). Either adduntagAllFolders/untagSelectedFoldersfunctions or remove the unused mutation.Also applies to: 203-203
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (5)
66-77: Avoid type assertion; extendFolderDetailsif needed.The
(f as any).taggingCompletedcast bypasses type safety. IftaggingCompletedis a valid property, add it to theFolderDetailstype definition.- if (percentage >= 100 || (f as any).taggingCompleted) { + if (percentage >= 100 || f.taggingCompleted) {Then update
FolderDetailsin@/types/Folder:export interface FolderDetails { // existing properties... taggingCompleted?: boolean; }
35-58: Selection state may contain stale folder IDs.When folders are deleted, their IDs remain in the
selectedstate. Consider filteringselectedIdsagainst current folder IDs or clearing selection on folder changes.const selectedIds = useMemo( () => Object.entries(selected) .filter(([, v]) => v) - .map(([id]) => id), - [selected], + .map(([id]) => id) + .filter((id) => folders.some((f) => f.folder_id === id)), + [selected, folders], );
340-345: Consider using ShadCN Checkbox for UI consistency.The native
<input type="checkbox">may look inconsistent with other ShadCN components. Consider using the Checkbox component from@/components/ui/checkbox.+import { Checkbox } from '@/components/ui/checkbox'; ... - <input - type="checkbox" - className="h-4 w-4 rounded border-gray-300" - checked={selected} - onChange={onToggleSelect} - /> + <Checkbox + checked={selected} + onCheckedChange={onToggleSelect} + />
164-170: Addaria-expandedfor accessibility.Collapsible sections should communicate their state to screen readers.
<Button variant="ghost" size="sm" onClick={() => setShowCompleted((v) => !v)} + aria-expanded={showCompleted} > {showCompleted ? 'Collapse' : 'Expand'} </Button>Apply similarly to the In Progress (Line 208) and Pending (Line 252) section buttons.
310-399: Consider movingFolderRowdefinition before its usage or to a separate file.
FolderRowis defined afterFolderManagementCardbut used inside it. While JavaScript hoisting makes this work, placing the helper component before its consumer or extracting it to a separate file improves readability.backend/DATABASE_MIGRATIONS.md (1)
6-8: Add language specification to code block.The code block should specify a language for proper syntax highlighting.
Apply this diff:
-``` +```plaintext [BACKEND] | ERROR | Error getting all images: no such column: i.isFavourite</blockquote></details> <details> <summary>backend/migrate_database.py (2)</summary><blockquote> `13-17`: **Use parameterized queries to prevent SQL injection.** While the current usage is safe (table_name is developer-controlled), using f-strings in SQL queries is not best practice and could become a vulnerability if the function is reused elsewhere. Consider using SQLite's identifier quoting: ```diff def check_column_exists(cursor, table_name: str, column_name: str) -> bool: """Check if a column exists in a table.""" - cursor.execute(f"PRAGMA table_info({table_name})") + # Note: PRAGMA commands don't support parameterized table names + # Validate table_name to prevent injection + if not table_name.replace('_', '').isalnum(): + raise ValueError(f"Invalid table name: {table_name}") + cursor.execute(f"PRAGMA table_info({table_name})") columns = [col[1] for col in cursor.fetchall()] return column_name in columns
53-55: Initialize logging for the migration script.The script uses
get_logger(__name__)but doesn't callsetup_logging(), which means it will use Python's default logging configuration. This may result in inconsistent log formatting.Apply this diff:
+from app.logging.setup_logging import get_logger, setup_logging + if __name__ == "__main__": + setup_logging("backend") print("Starting database migration...") migrate_database()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
backend/DATABASE_MIGRATIONS.md(1 hunks)backend/app/logging/setup_logging.py(1 hunks)backend/migrate_database.py(1 hunks)docs/backend/backend_python/openapi.json(1 hunks)frontend/src/hooks/useFolderOperations.tsx(2 hunks)frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T16:40:22.622Z
Learnt from: Hemil36
Repo: AOSSIE-Org/PictoPy PR: 548
File: backend/app/logging/setup_logging.py:224-243
Timestamp: 2025-10-13T16:40:22.622Z
Learning: In the PictoPy project's centralized logging system (backend/app/logging/setup_logging.py and sync-microservice/app/logging/setup_logging.py), stack traces should not be shown in logs. When rerouting logs through InterceptHandler, exc_info and stack_info should not be preserved.
Applied to files:
backend/app/logging/setup_logging.py
🧬 Code graph analysis (2)
frontend/src/hooks/useFolderOperations.tsx (2)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/folders.ts (2)
enableAITagging(44-52)disableAITagging(54-62)
backend/migrate_database.py (1)
backend/app/logging/setup_logging.py (2)
setup_logging(134-196)get_logger(199-209)
🪛 markdownlint-cli2 (0.18.1)
backend/DATABASE_MIGRATIONS.md
6-6: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: Backend Tests
- 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)
🔇 Additional comments (3)
backend/app/logging/setup_logging.py (1)
238-248: LGTM! Robust recursive interception guard.The
_interceptedattribute guard effectively prevents infinite loops when log records are re-routed through the root logger. The direct routing to root logger with the[uvicorn]prefix is clean and maintains the centralized logging approach.Based on learnings, the implementation correctly avoids preserving
exc_infoandstack_info, which aligns with the project's logging standards.docs/backend/backend_python/openapi.json (1)
1120-1127: LGTM! Minor schema refinements with bug fix.The wrapping of the schema reference in
allOfis semantically equivalent but may be a tool-specific requirement. The removal of the trailing period from the default value"path."→"path"is a good fix.These changes appear to be auto-generated and are backward compatible.
backend/DATABASE_MIGRATIONS.md (1)
23-23: All referenced files and modules exist and are correctly imported.reset_database.pyis present atbackend/reset_database.py, theapp.database.imagesmodule exists atbackend/app/database/images.py, and thedb_create_images_table()function is defined in the module. No action needed.
| def migrate_database(): | ||
| """Add missing columns to the images table.""" | ||
| try: | ||
| conn = sqlite3.connect(DATABASE_PATH) | ||
| cursor = conn.cursor() | ||
|
|
||
| # Check if isFavourite column exists | ||
| if not check_column_exists(cursor, "images", "isFavourite"): | ||
| logger.info("Adding 'isFavourite' column to images table...") | ||
| cursor.execute( | ||
| """ | ||
| ALTER TABLE images | ||
| ADD COLUMN isFavourite BOOLEAN DEFAULT 0 | ||
| """ | ||
| ) | ||
| conn.commit() | ||
| logger.info("✓ Successfully added 'isFavourite' column") | ||
| else: | ||
| logger.info("✓ 'isFavourite' column already exists") | ||
|
|
||
| conn.close() | ||
| print("\n✅ Database migration completed successfully!") | ||
|
|
||
| except sqlite3.Error as e: | ||
| logger.error(f"Database migration failed: {e}") | ||
| print(f"\n❌ Migration failed: {e}") | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Unexpected error during migration: {e}") | ||
| print(f"\n❌ Unexpected error: {e}") | ||
| raise |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Resource leak: database connection not closed on exception.
If an exception occurs before line 40, the database connection remains open. This could lead to connection leaks and database locks.
Apply this diff to use a context manager:
def migrate_database():
"""Add missing columns to the images table."""
+ conn = None
try:
conn = sqlite3.connect(DATABASE_PATH)
cursor = conn.cursor()
# Check if isFavourite column exists
if not check_column_exists(cursor, "images", "isFavourite"):
logger.info("Adding 'isFavourite' column to images table...")
cursor.execute(
"""
ALTER TABLE images
ADD COLUMN isFavourite BOOLEAN DEFAULT 0
"""
)
conn.commit()
logger.info("✓ Successfully added 'isFavourite' column")
else:
logger.info("✓ 'isFavourite' column already exists")
- conn.close()
print("\n✅ Database migration completed successfully!")
except sqlite3.Error as e:
logger.error(f"Database migration failed: {e}")
print(f"\n❌ Migration failed: {e}")
raise
except Exception as e:
logger.error(f"Unexpected error during migration: {e}")
print(f"\n❌ Unexpected error: {e}")
raise
+ finally:
+ if conn:
+ conn.close()🤖 Prompt for AI Agents
In backend/migrate_database.py around lines 20 to 50, the sqlite3 connection is
opened but not guaranteed to be closed if an exception occurs; replace the
manual open/close with a context manager by using "with
sqlite3.connect(DATABASE_PATH) as conn:" and create the cursor inside that
block, perform ALTER and conn.commit() as needed, remove the explicit
conn.close(), and keep the existing exception handlers (they will still work) so
the connection is always closed even on errors.
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (2)
74-97: Consider passing pre-calculated progress to FolderRow.The grouping logic correctly enriches each folder with a
progressproperty, but this value is recalculated insideFolderRow(line 358) rather than being reused. This results in redundant calls togetFolderProgress.Pass the pre-calculated
progresstoFolderRowto eliminate redundant computation:const FolderRow: React.FC<{ folder: FolderDetails; + progress: number; taggingStatus?: { tagging_percentage?: number }; selected: boolean; // ... other props }> = ({ folder, + progress, taggingStatus, // ... other params }) => { - const percentage = getFolderProgress(folder, taggingStatus); + const percentage = progress; const isComplete = percentage >= 100;Then update the calls at lines 200-217, 244-261, and 288-305:
-<FolderRow - key={folder.folder_id} - folder={folder} - taggingStatus={taggingStatus[folder.folder_id]} +<FolderRow + key={folder.folder_id} + folder={folder} + progress={folder.progress} + taggingStatus={taggingStatus[folder.folder_id]}
182-315: Consider extracting a reusable Section component.The three collapsible sections (Completed, In Progress, Pending) follow an identical pattern with only the title, visibility state, and data source varying. Extracting a reusable
CollapsibleSectioncomponent would reduce duplication and improve maintainability.Example extraction:
const CollapsibleSection: React.FC<{ title: string; emptyMessage: string; folders: Array<FolderDetails & { progress: number }>; isExpanded: boolean; onToggle: () => void; // ... other shared props }> = ({ title, emptyMessage, folders, isExpanded, onToggle, ... }) => ( <div className="space-y-2"> <div className="flex items-center justify-between"> <h4 className="text-foreground text-sm font-semibold">{title}</h4> <Button variant="ghost" size="sm" onClick={onToggle}> {isExpanded ? 'Collapse' : 'Expand'} </Button> </div> {isExpanded && ( <div className="space-y-3"> {folders.map((folder) => ( <FolderRow key={folder.folder_id} /* ... */ /> ))} {folders.length === 0 && ( <div className="text-muted-foreground text-xs">{emptyMessage}</div> )} </div> )} </div> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (4)
frontend/src/hooks/useFolderOperations.tsx (1)
useFolderOperations(20-205)frontend/src/app/store.ts (1)
RootState(22-22)frontend/src/components/ui/progress.tsx (1)
Progress(37-37)frontend/src/components/ui/switch.tsx (1)
Switch(29-29)
⏰ 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 (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (ubuntu-22.04)
🔇 Additional comments (5)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (5)
15-27: LGTM!The progress calculation correctly handles all cases: completed folders return 100%, active folders return their clamped percentage, and folders without status return 0%.
49-72: LGTM!Selection state management is well-structured with proper memoization. The
allSelectedandselectedIdsderivations correctly track state changes.
99-123: LGTM!The totals calculation correctly derives all metrics from the grouped data. The progress percentage represents the average completion across all folders, which aligns with the PR objectives.
138-180: LGTM!Bulk controls are properly wired with correct disable logic during pending operations. The progress summary displays all required metrics per the PR objectives.
419-419: Verify thatindicatorClassNameis a supported prop on the Progress component.The
indicatorClassNameprop is passed to theProgresscomponent at line 419, but this needs verification against the component's prop interface to confirm it's a supported attribute. Check the Progress component definition to ensure the prop is properly defined and won't be silently ignored.
| </div> | ||
| {showCompleted && ( | ||
| <div className="space-y-3"> | ||
| {groups.completed.map((folder: FolderDetails) => ( |
There was a problem hiding this comment.
Remove explicit type annotations from map callbacks.
The explicit (folder: FolderDetails) type annotations on lines 200, 244, and 288 narrow the inferred type from FolderDetails & { progress: number } to just FolderDetails, which discards the progress property. Since TypeScript correctly infers the type, the explicit annotation is unnecessary and potentially misleading.
Apply this diff to rely on type inference:
-{groups.completed.map((folder: FolderDetails) => (
+{groups.completed.map((folder) => (-{groups.inProgress.map((folder: FolderDetails) => (
+{groups.inProgress.map((folder) => (-{groups.pending.map((folder: FolderDetails) => (
+{groups.pending.map((folder) => (Also applies to: 244-244, 288-288
🤖 Prompt for AI Agents
In frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx around
lines 200, 244, and 288, the map callbacks use explicit parameter types like
(folder: FolderDetails) which narrow the inferred type and drop the progress
property; remove the explicit type annotations from those map callback
parameters so TypeScript can infer FolderDetails & { progress: number } (e.g.,
change to folder => ...) and leave the rest of the callback body unchanged.
| <input | ||
| type="checkbox" | ||
| className="h-4 w-4 rounded border-gray-300" | ||
| checked={selected} | ||
| onChange={onToggleSelect} | ||
| /> |
There was a problem hiding this comment.
Add accessible label to selection checkbox.
The selection checkbox lacks an associated label or aria-label, making it inaccessible to screen reader users who won't know its purpose.
Apply this diff to add an accessible label:
<input
type="checkbox"
className="h-4 w-4 rounded border-gray-300"
checked={selected}
onChange={onToggleSelect}
+ aria-label={`Select folder ${folder.folder_path}`}
/>📝 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.
| <input | |
| type="checkbox" | |
| className="h-4 w-4 rounded border-gray-300" | |
| checked={selected} | |
| onChange={onToggleSelect} | |
| /> | |
| <input | |
| type="checkbox" | |
| className="h-4 w-4 rounded border-gray-300" | |
| checked={selected} | |
| onChange={onToggleSelect} | |
| aria-label={`Select folder ${folder.folder_path}`} | |
| /> |
🤖 Prompt for AI Agents
In frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx around
lines 366 to 371, the checkbox input is missing an accessible label; add an
explicit accessible name by either wrapping the checkbox in a visible <label>
that references the input (using htmlFor/id) or by adding an appropriate
aria-label or aria-labelledby that describes the checkbox purpose (e.g., "Select
folder" or include folder name). Ensure the id used is unique if using htmlFor,
and keep the label text concise and descriptive so screen readers convey the
checkbox purpose.
|
|
|
Hey @rahulharpal1603, pls review my pr, and suggest me if any changes are required |
1 participant
…gress summary
Problem
fixes: #755
Managing AI tagging for multiple folders was tedious:
This creates significant friction when managing 10+ folders.
Solution
I’ve implemented a complete overhaul of the folder management UI with:
Bulk Control Actions
Smart Sorting by Status
Folders are now auto-grouped into collapsible sections:
Real-Time Progress Summary
Clear stats at the top:
Technical Highlights
📎 Media
💡 Why This Matters
This change transforms folder management from a chore into a smooth, at-a-glance experience—especially valuable for users with large photo libraries. It aligns perfectly with PictoPy’s goal of smart, privacy-first gallery management.
Ready for review! 🙌
Recording.2025-12-16.052735.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
API
✏️ Tip: You can customize this high-level summary in your review settings.