Feat/total image count #1010
Feat/total image count #1010
Conversation
📝 WalkthroughWalkthroughThe PR implements a feature to display the total number of images across the entire gallery. It adds a database function to retrieve the total image count, enriches the Changes
Sequence Diagram(s)sequenceDiagram
participant DB as Database
participant API as Backend API
participant Hook as useFolderOperations
participant Redux as Redux Store
participant UI as FolderManagementCard
API->>DB: SELECT COUNT(*) FROM images
DB-->>API: total_count
Hook->>API: GET /all-folders
API-->>Hook: {folders, total_count, total_images}
Hook->>Redux: dispatch setFolders(folders)
Hook->>Redux: dispatch setTotalImages(total_images)
Redux-->>UI: totalImages state update
UI->>UI: render with total image count
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In @backend/app/routes/folders.py:
- Around line 14-15: The code calls db_get_total_image_count for the
/all-folders endpoint and returns 0 on DB error, which is both a potential perf
hotspot (COUNT(*) on every request) and hides errors; change the implementation
to avoid per-request full-table COUNT(*) by either: 1) returning the
total_images as a nullable field or propagating the DB error upward instead of
defaulting to 0 (so the UI can handle/error), or 2) compute the count alongside
your existing folder query or use a cached/materialized value updated
asynchronously; update the handler(s) that reference db_get_total_image_count
(including the other use at the block around lines 464-472) to adopt one of
these approaches and ensure error paths do not silently return 0.
In @frontend/package.json:
- Line 91: The devDependency "baseline-browser-mapping" is unused and should be
removed from frontend/package.json; delete the "baseline-browser-mapping":
"^2.9.11" entry from the devDependencies block, run your package manager (npm
install or yarn install) to update the lockfile, and verify there are no
references by searching the repo for "baseline-browser-mapping" and confirming
it no longer appears in package-lock.json or yarn.lock.
In @frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx:
- Around line 29-55: Format the totalImages displayed in the SettingsCard
description by calling totalImages.toLocaleString() so it renders like "1,250
images total"; switch the Button's onClick to the functional updater form
setIsExpanded(prev => !prev) and add an aria-label (e.g., aria-label="Toggle
folder management") to the icon-only expand/collapse Button for accessibility;
in the folder list rendering replace unstable index keys with the stable
folder.folder_id property (use folder.folder_id as the key wherever you
currently use index).
🧹 Nitpick comments (1)
frontend/src/hooks/useFolderOperations.tsx (1)
11-12: Remove theunknowncast; readtotal_imagesvia the typed response (?? 0)
The current cast can hide contract mismatches and makes refactors risky. Prefer accessing the field directly and using nullish coalescing.Proposed diff
useEffect(() => { if (foldersQuery.data?.data?.folders) { const folders = foldersQuery.data.data.folders as FolderDetails[]; dispatch(setFolders(folders)); - dispatch( - setTotalImages( - (foldersQuery.data.data as unknown as { total_images: number }) - .total_images || 0, - ), - ); + dispatch(setTotalImages(foldersQuery.data.data.total_images ?? 0)); } }, [foldersQuery.data, dispatch]);Also applies to: 60-71
📜 Review details
Configuration used: defaults
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 (10)
backend/app/database/images.pybackend/app/routes/folders.pybackend/app/schemas/folders.pydocs/backend/backend_python/openapi.jsonfrontend/package.jsonfrontend/src/features/folderSlice.tsfrontend/src/hooks/useFolderOperations.tsxfrontend/src/pages/SettingsPage/components/FolderManagementCard.tsxfrontend/src/pages/SettingsPage/components/SettingsCard.tsxfrontend/src/types/Folder.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/hooks/useFolderOperations.tsx (1)
frontend/src/features/folderSlice.ts (1)
setTotalImages(28-30)
backend/app/routes/folders.py (3)
backend/app/database/images.py (1)
db_get_total_image_count(424-440)backend/app/schemas/folders.py (2)
GetAllFoldersResponse(66-70)GetAllFoldersData(35-38)frontend/src/types/Folder.ts (1)
GetAllFoldersData(10-14)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (3)
frontend/src/app/store.ts (1)
RootState(22-22)frontend/src/types/Folder.ts (1)
FolderDetails(1-8)frontend/src/components/FolderPicker/FolderPicker.tsx (1)
FolderPicker(5-29)
🔇 Additional comments (6)
docs/backend/backend_python/openapi.json (1)
1789-1798: LGTM!The OpenAPI schema correctly reflects the addition of the
total_imagesfield toGetAllFoldersData, with appropriate type (integer) and required status.backend/app/schemas/folders.py (1)
38-38: LGTM!The addition of
total_images: intto theGetAllFoldersDatamodel is straightforward and consistent with the existing schema structure.frontend/src/types/Folder.ts (1)
13-13: LGTM!The TypeScript interface correctly mirrors the backend schema change, with appropriate type mapping (Python
int→ TypeScriptnumber).backend/app/database/images.py (1)
424-440: Consider the error handling approach, though consistent with existing patterns.The function returns
0on both database errors and when no results exist, which masks failures. However, this approach is consistent with other database functions in this file (e.g.,db_get_all_imagesreturns[]on error). Since errors are logged and this is for display purposes, the current implementation is acceptable.For enhanced robustness, consider whether the calling code in
backend/app/routes/folders.pyshould handle potential database unavailability differently (e.g., returning an error response vs. showing 0 images).frontend/src/pages/SettingsPage/components/SettingsCard.tsx (1)
17-21: Action slot addition looks good; ensure icon-only actions are accessible
action?: React.ReactNodeand theml-autoplacement are clean and flexible. Just make sure consumers (e.g., icon-only<Button>) provide anaria-label/tooltip since the card can’t enforce that.Also applies to: 34-35, 47-47
frontend/src/features/folderSlice.ts (1)
5-16: Redux addition is clean and consistent with the API contract
totalImages+setTotalImages, and resetting it inclearFolders(), all look good. (Optional: ifclearFoldersis meant to fully reset state, also consider clearinglastUpdatedAt.)Also applies to: 27-31, 77-80, 100-109
| from app.database.images import db_get_total_image_count | ||
| from app.logging.setup_logging import get_logger |
There was a problem hiding this comment.
Consider perf + error semantics of COUNT(*) on every /all-folders call
This correctly wires total_images into the response, but a full-table COUNT(*) can become expensive at scale, and returning 0 on error (per db_get_total_image_count) can silently mislead the UI. Consider either caching/materializing the count (or computing it alongside existing queries), and/or propagating an error (or a nullable field) instead of defaulting to 0 on DB failure.
Also applies to: 464-472
🤖 Prompt for AI Agents
In @backend/app/routes/folders.py around lines 14 - 15, The code calls
db_get_total_image_count for the /all-folders endpoint and returns 0 on DB
error, which is both a potential perf hotspot (COUNT(*) on every request) and
hides errors; change the implementation to avoid per-request full-table COUNT(*)
by either: 1) returning the total_images as a nullable field or propagating the
DB error upward instead of defaulting to 0 (so the UI can handle/error), or 2)
compute the count alongside your existing folder query or use a
cached/materialized value updated asynchronously; update the handler(s) that
reference db_get_total_image_count (including the other use at the block around
lines 464-472) to adopt one of these approaches and ensure error paths do not
silently return 0.
| "@vitejs/plugin-react": "^4.2.1", | ||
| "autoprefixer": "^10.4.20", | ||
| "babel-jest": "^29.7.0", | ||
| "baseline-browser-mapping": "^2.9.11", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the package.json file to confirm the dependency
cat -n frontend/package.json | head -100Repository: AOSSIE-Org/PictoPy
Length of output: 4146
🏁 Script executed:
# Search for any usage of baseline-browser-mapping in the codebase
rg "baseline-browser-mapping" --type js --type json -A 2 -B 2Repository: AOSSIE-Org/PictoPy
Length of output: 1775
🏁 Script executed:
# Also search for any imports or references to this package
rg "baseline.*browser.*mapping" --type js --type ts --type jsonRepository: AOSSIE-Org/PictoPy
Length of output: 563
🏁 Script executed:
# Check what packages depend on baseline-browser-mapping
rg "baseline-browser-mapping" frontend/package-lock.json -B 20 | grep -E '"(name|requires|dependencies)"' | head -30Repository: AOSSIE-Org/PictoPy
Length of output: 85
🏁 Script executed:
# Search for any configuration files that might use it
find frontend -type f \( -name "jest.config.*" -o -name "postcss.config.*" -o -name "babel.config.*" -o -name ".babelrc*" \) -exec cat {} \;Repository: AOSSIE-Org/PictoPy
Length of output: 753
🏁 Script executed:
# Check if it's referenced in any scripts or configuration
rg "baseline" frontend --type json --type js --type ts -iRepository: AOSSIE-Org/PictoPy
Length of output: 563
🏁 Script executed:
# Check which package in package-lock.json has baseline-browser-mapping as a dependency
python3 <<'EOF'
import json
import sys
with open('frontend/package-lock.json', 'r') as f:
lock_data = json.load(f)
packages = lock_data.get('packages', {})
for pkg_name, pkg_info in packages.items():
if 'dependencies' in pkg_info:
if 'baseline-browser-mapping' in pkg_info['dependencies']:
print(f"Package '{pkg_name}' depends on baseline-browser-mapping: {pkg_info['dependencies']['baseline-browser-mapping']}")
EOFRepository: AOSSIE-Org/PictoPy
Length of output: 148
🏁 Script executed:
# Also check the dependencies of autoprefixer and caniuse which often pull in browser mappings
rg "\"autoprefixer\"" frontend/package-lock.json -A 15 | head -40Repository: AOSSIE-Org/PictoPy
Length of output: 1193
Remove baseline-browser-mapping from devDependencies
This package is explicitly listed but has no usage anywhere in the codebase—not in any code files, configuration files, or npm scripts. It's a CLI tool for browser mapping that's unrelated to the PR objective of displaying total image count and appears to be an accidental inclusion.
🤖 Prompt for AI Agents
In @frontend/package.json at line 91, The devDependency
"baseline-browser-mapping" is unused and should be removed from
frontend/package.json; delete the "baseline-browser-mapping": "^2.9.11" entry
from the devDependencies block, run your package manager (npm install or yarn
install) to update the lockfile, and verify there are no references by searching
the repo for "baseline-browser-mapping" and confirming it no longer appears in
package-lock.json or yarn.lock.
| const [isExpanded, setIsExpanded] = useState(true); | ||
|
|
||
| const taggingStatus = useSelector( | ||
| (state: RootState) => state.folders.taggingStatus, | ||
| ); | ||
|
|
||
| const totalImages = useSelector((state: RootState) => state.folders.totalImages); | ||
|
|
||
| return ( | ||
| <SettingsCard | ||
| icon={Folder} | ||
| title="Folder Management" | ||
| description="Configure your photo library folders and AI settings" | ||
| description={`Configure your photo library folders and AI settings (${totalImages} images total)`} | ||
| action={ | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-8 w-8 p-0" | ||
| onClick={() => setIsExpanded(!isExpanded)} | ||
| > | ||
| {isExpanded ? ( | ||
| <ChevronUp className="h-4 w-4" /> | ||
| ) : ( | ||
| <ChevronDown className="h-4 w-4" /> | ||
| )} | ||
| </Button> | ||
| } |
There was a problem hiding this comment.
Fix formatting + list keys + a11y for the new header action
- Format
totalImages(toLocaleString) to match the “1,250 images total” requirement. - Use a stable key (
folder.folder_id) instead ofindex. - Add
aria-labelfor the icon-only expand/collapse button; use functional state update.
Proposed diff
- const [isExpanded, setIsExpanded] = useState(true);
+ const [isExpanded, setIsExpanded] = useState(true);
const taggingStatus = useSelector(
(state: RootState) => state.folders.taggingStatus,
);
- const totalImages = useSelector((state: RootState) => state.folders.totalImages);
+ const totalImages = useSelector((state: RootState) => state.folders.totalImages);
+ const totalImagesLabel = totalImages.toLocaleString();
return (
<SettingsCard
icon={Folder}
title="Folder Management"
- description={`Configure your photo library folders and AI settings (${totalImages} images total)`}
+ description={`Configure your photo library folders and AI settings (${totalImagesLabel} images total)`}
action={
<Button
variant="ghost"
size="sm"
className="h-8 w-8 p-0"
- onClick={() => setIsExpanded(!isExpanded)}
+ aria-label={isExpanded ? 'Collapse folder management' : 'Expand folder management'}
+ onClick={() => setIsExpanded((v) => !v)}
>
{isExpanded ? (
<ChevronUp className="h-4 w-4" />
) : (
<ChevronDown className="h-4 w-4" />
)}
</Button>
}
>
{isExpanded && (
<div className="animate-in fade-in slide-in-from-top-2 duration-200">
{folders.length > 0 ? (
<div className="space-y-3">
{folders.map((folder: FolderDetails, index: number) => (
<div
- key={index}
+ key={folder.folder_id}
className="group border-border bg-background/50 relative rounded-lg border p-4 transition-all hover:border-gray-300 hover:shadow-sm dark:hover:border-gray-600"
>Also applies to: 35-35, 41-41, 57-66, 61-65
🤖 Prompt for AI Agents
In @frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx around
lines 29 - 55, Format the totalImages displayed in the SettingsCard description
by calling totalImages.toLocaleString() so it renders like "1,250 images total";
switch the Button's onClick to the functional updater form setIsExpanded(prev =>
!prev) and add an aria-label (e.g., aria-label="Toggle folder management") to
the icon-only expand/collapse Button for accessibility; in the folder list
rendering replace unstable index keys with the stable folder.folder_id property
(use folder.folder_id as the key wherever you currently use index).
1 participant
Fixes #992
Implementation Approach
1. Backend (The Logic)
Database Count
Updating the API
GET /all-foldersendpoint. Previously, it only returned a list of folders. Now, before sending the response, it calls my new database function to get the total count and packages it alongside the folder list.Data Structure
total_images.2. Frontend (The Visuals)
Updating Definitions
total_imagesnumber from the server.Storing the Data
Displaying it
In short: The backend counts the files, sends the number with the folder list, and the frontend grabs that number and simply displays it in the settings card.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.