feat: implement manual cluster management system#1196
Open
Arunodoy18 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Open
feat: implement manual cluster management system#1196Arunodoy18 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Arunodoy18 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
Backend: - Add manual_clusters + manual_cluster_images tables (SQLite, FK cascade) - Service layer with ClusterNotFoundError / ImageNotFoundError exceptions - RESTful endpoints: POST/GET/PATCH/DELETE /clusters, bulk assign/remove images - 18 unit tests covering all CRUD paths and edge cases - Register tables and router in main.py Frontend: - ManualCluster TypeScript types - Axios API functions for all 7 endpoints - Redux slice (manualClustersSlice) with optimistic remove - useManualClusters custom hook (loading/error state, rollback on failure) - ClusterCard component (inline rename, delete) - ImagePickerModal (paginated 40/page, debounced search, multi-select) - ClusterListPage (/clusters) and ClusterDetailPage (/clusters/:id) - My Clusters nav item in sidebar - New CLUSTERS + CLUSTER_DETAIL routes
Contributor
|
|
Contributor
WalkthroughThis pull request introduces a complete manual cluster management feature spanning backend and frontend. It adds database persistence, CRUD service operations, RESTful API endpoints, Redux state management, UI components, and page routes to enable users to create, organize, and manage image clusters. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Frontend
participant Server as Backend API
participant DB as SQLite Database
User->>Client: Click "Create Cluster"
Client->>Server: POST /clusters/ {name}
Server->>DB: INSERT cluster record
DB-->>Server: Return cluster_id, timestamps
Server-->>Client: 201 {cluster}
Client->>Client: Redux: addCluster()
Client-->>User: Show new cluster in list
User->>Client: Click "My Clusters"
Client->>Server: GET /clusters/
Server->>DB: SELECT all clusters with image_count
DB-->>Server: Return cluster rows
Server-->>Client: 200 {clusters[]}
Client->>Client: Redux: setClusters()
Client-->>User: Display cluster grid
sequenceDiagram
actor User
participant Client as Frontend
participant Server as Backend API
participant DB as SQLite Database
User->>Client: Click cluster, open ImagePickerModal
Client->>Client: Display available images
User->>Client: Select images, click "Assign"
Client->>Server: POST /clusters/{id}/images {image_ids}
Server->>DB: Validate image_ids exist
DB-->>Server: Confirmation
Server->>DB: INSERT image mappings (OR IGNORE)
DB-->>Server: Return assigned/skipped counts
Server-->>Client: 200 {assigned_count, skipped_count}
Client->>Client: Redux: appendImages()
Client-->>User: Update cluster detail with new images
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Contributor
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (11)
backend/tests/test_manual_clusters.py (1)
143-167: Consider adding a test for rename conflict (409) once the route handler is updated.If
DuplicateClusterNameErrorhandling is added to the rename endpoint (as flagged in the routes review), a corresponding test should accompany it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_manual_clusters.py` around lines 143 - 167, Add a new test method in the TestRenameCluster class (e.g., test_rename_conflict) that patches app.routes.manual_clusters.rename_cluster to raise DuplicateClusterNameError and then calls the PATCH /clusters/{id} endpoint with the conflicting name; assert the response.status_code is 409 and the body indicates conflict. Reference the DuplicateClusterNameError exception and the rename_cluster patch used in other tests to implement this test consistently with test_rename_missing_cluster.frontend/src/pages/Clusters/ClusterDetailPage.tsx (3)
42-45: Same eslint-disable pattern as ClusterListPage —loadClusterDetailcan safely be added to the dep array.
loadClusterDetailis memoized withuseCallback([dispatch]), so including it won't cause extra renders. Also,clusterIdis already in the dep array, which is correct.♻️ Suggested change
useEffect(() => { if (clusterId) loadClusterDetail(clusterId); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [clusterId]); + }, [clusterId, loadClusterDetail]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Clusters/ClusterDetailPage.tsx` around lines 42 - 45, The effect currently silences eslint for missing deps; update the useEffect for loading details to include loadClusterDetail in the dependency array (i.e., useEffect(() => { if (clusterId) loadClusterDetail(clusterId); }, [clusterId, loadClusterDetail])) since loadClusterDetail is already memoized via useCallback([dispatch]) and won't cause extra renders; keep clusterId in the deps as well and remove the eslint-disable comment.
47-55: Consider usingsuccessDatafromusePictoQueryinstead of double-nestingdata?.data.The
usePictoQueryhook already provides asuccessDatafield (as seen infrontend/src/hooks/useQueryExtension.ts) that extracts the nesteddataproperty. Using it would be cleaner and more consistent with the hook's intended API.♻️ Suggested change
- const { data: allImagesData } = usePictoQuery({ + const { successData } = usePictoQuery({ queryKey: ['all-images-for-picker'], queryFn: () => fetchAllImages(), }); const allImages: Image[] = useMemo( - () => (allImagesData?.data as Image[]) ?? [], - [allImagesData], + () => (successData as Image[]) ?? [], + [successData], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Clusters/ClusterDetailPage.tsx` around lines 47 - 55, The code is accessing nested response data via allImagesData?.data instead of using the hook's normalized success payload; update the use of usePictoQuery to read from its successData field (provided by useQueryExtension) — replace references to allImagesData and the memo that derives allImages from allImagesData?.data with successData (e.g., const { successData: allImagesSuccess } = usePictoQuery(...) and derive allImages from that) while keeping the existing queryKey and queryFn (fetchAllImages) and preserving the allImages: Image[] type and useMemo wrapper.
290-294: Use Tailwind'saspect-squareclass instead of inline style.♻️ Suggested change
<div - className="group relative overflow-hidden rounded-md border" - style={{ aspectRatio: '1/1' }} + className="group relative aspect-square overflow-hidden rounded-md border" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Clusters/ClusterDetailPage.tsx` around lines 290 - 294, Replace the inline style on the JSX container with Tailwind's utility: in the ClusterDetailPage component update the returned <div> (the element with className "group relative overflow-hidden rounded-md border") to remove the style={{ aspectRatio: '1/1' }} prop and add the Tailwind class "aspect-square" to its className so it becomes "group relative overflow-hidden rounded-md border aspect-square".frontend/src/components/Clusters/ClusterCard.tsx (1)
48-50: Hardcoded navigation path.The path
/clusters/${cluster.cluster_id}duplicates the route definition fromROUTES.CLUSTER_DETAIL. Consider deriving it from the constant to avoid drift if the route changes. The same pattern appears inClusterDetailPage.tsx(lines 85, 130, 147).♻️ Example
+import { ROUTES } from '@/constants/routes'; + const handleCardClick = () => { - if (!editing) navigate(`/clusters/${cluster.cluster_id}`); + if (!editing) navigate(`/${ROUTES.CLUSTERS}/${cluster.cluster_id}`); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Clusters/ClusterCard.tsx` around lines 48 - 50, Replace the hardcoded path in handleCardClick with the ROUTES constant: instead of navigate(`/clusters/${cluster.cluster_id}`) use the ROUTES.CLUSTER_DETAIL template (or a helper that formats it) to build the URL using cluster.cluster_id; update the call in ClusterCard's handleCardClick and mirror the same change where the literal path is used in ClusterDetailPage (the occurrences around the handlers at lines referenced) so all navigation uses ROUTES.CLUSTER_DETAIL with the id parameter rather than a duplicated string.backend/app/routes/manual_clusters.py (1)
201-201: Inconsistent use ofPath(...)annotation.
rename_cluster_endpointandassign_images_endpointdeclarecluster_id: strwithoutPath(...), while other endpoints (lines 166, 235, 310) usePath(...). This inconsistency doesn't affect functionality (FastAPI infers path params), but is worth harmonizing for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/routes/manual_clusters.py` at line 201, The cluster_id parameter is inconsistently annotated across endpoints; update the signature of rename_cluster_endpoint (and similarly assign_images_endpoint) to use FastAPI's Path(...) annotation for cluster_id so it matches other handlers (e.g., the endpoints at lines referenced that already use Path(...)); change the parameter declaration to accept cluster_id: str = Path(..., description="cluster id") (or equivalent) to harmonize annotations and include an explanatory description if present in other handlers.frontend/src/pages/Clusters/ClusterListPage.tsx (2)
32-35: Consider includingloadClustersin the dependency array instead of suppressing the lint rule.Since
loadClustersis memoized withuseCallbackand depends only ondispatch(which is stable), adding it to the dep array is safe and eliminates the need for the eslint-disable comment.♻️ Suggested change
useEffect(() => { loadClusters(); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [loadClusters]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Clusters/ClusterListPage.tsx` around lines 32 - 35, Replace the eslint suppression by including the memoized loadClusters function in the useEffect dependency array: update the useEffect that currently calls loadClusters() so its deps include loadClusters, and remove the "// eslint-disable-next-line react-hooks/exhaustive-deps" comment; loadClusters is defined with useCallback and only depends on dispatch so it is stable and safe to add as a dependency.
119-126: Dialog can be dismissed while creation is in progress.The
onOpenChangehandler doesn't checkcreating, so a user can close the dialog via Escape or clicking outside while the API call is in flight. The Cancel button is correctly disabled, but the dialog itself is not guarded. Consider adding a guard:♻️ Suggested change
<Dialog open={showCreate} onOpenChange={(v) => { - if (!v) { + if (!v && !creating) { setShowCreate(false); setNewName(''); } }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Clusters/ClusterListPage.tsx` around lines 119 - 126, The Dialog's onOpenChange currently closes the modal and resets state even when a create request is in flight; update the onOpenChange handler (the function passed to Dialog) to guard against closing when the creating flag is true by returning early if v is false and creating is true, and only call setShowCreate(false) and setNewName('') when not creating; reference Dialog's onOpenChange, the creating variable, setShowCreate and setNewName to locate where to add the guard.frontend/src/features/manualClustersSlice.ts (1)
51-69:appendImagesreducer is unused and should be removed or documented.The reducer is exported from
manualClustersSlice.tsbut never imported or dispatched anywhere in the codebase. InuseManualClusters.ts, theassignImagesfunction reloads the full cluster detail viasetClusterDetailafter the API call (lines 172–176) rather than using the incrementalappendImagesreducer.If this reducer is intended for future optimizations, add a brief comment explaining its purpose. Otherwise, remove it to keep the slice surface area minimal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/manualClustersSlice.ts` around lines 51 - 69, The appendImages reducer in manualClustersSlice.ts is unused (no imports/dispatches) while useManualClusters.ts's assignImages calls setClusterDetail after the API update; either remove appendImages to shrink the slice surface or keep it but add a short TODO comment explaining its purpose (e.g., intended for incremental client-side updates to avoid full reloads) and reference where it would be used (assignImages / setClusterDetail) so future devs know why it exists; update exports accordingly (remove from export list if deleting, or leave exported with the explanatory comment if retaining).frontend/src/hooks/useManualClusters.ts (1)
35-35:useDispatch()should use the typedAppDispatch.Using the untyped
useDispatch()loses type-safety for dispatched actions. Create and use a typed hook as is standard practice in Redux Toolkit projects.♻️ Proposed fix
-import { useDispatch, useSelector } from 'react-redux'; +import { useDispatch, useSelector } from 'react-redux'; +import type { AppDispatch } from '@/app/store'; export function useManualClusters() { - const dispatch = useDispatch(); + const dispatch = useDispatch<AppDispatch>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useManualClusters.ts` at line 35, The code uses the untyped useDispatch() in useManualClusters.ts which loses action type-safety; create a typed hook useAppDispatch that returns useDispatch<AppDispatch>() (or add/consume an existing useAppDispatch) and replace the raw useDispatch() call with useAppDispatch(); import AppDispatch from your store types if needed and update the declaration `const dispatch = useDispatch();` to `const dispatch = useAppDispatch();` so dispatched actions in useManualClusters are properly typed.backend/app/database/manual_clusters.py (1)
207-231: Loop-based insert indb_add_images_to_manual_cluster— useexecutemanyfor better performance.Iterating Python-side to detect inserted rows is the only reason for the loop. An
executemanyfor the bulk insert, combined with a pre-query to find already-assigned ids, would be faster on large batches and reduce per-row overhead.♻️ Proposed refactor
def db_add_images_to_manual_cluster(cluster_id: str, image_ids: List[str]) -> List[str]: - inserted: List[str] = [] with get_db_connection() as conn: - for image_id in image_ids: - cursor = conn.execute( - """ - INSERT OR IGNORE INTO manual_cluster_images (cluster_id, image_id) - VALUES (?, ?) - """, - (cluster_id, image_id), - ) - if cursor.rowcount > 0: - inserted.append(image_id) + # Find already-assigned images before bulk insert + placeholders = ",".join("?" * len(image_ids)) + existing_cursor = conn.execute( + f"SELECT image_id FROM manual_cluster_images " + f"WHERE cluster_id = ? AND image_id IN ({placeholders})", + [cluster_id, *image_ids], + ) + already_assigned = {row[0] for row in existing_cursor.fetchall()} + inserted = [iid for iid in image_ids if iid not in already_assigned] + conn.executemany( + "INSERT OR IGNORE INTO manual_cluster_images (cluster_id, image_id) VALUES (?, ?)", + [(cluster_id, iid) for iid in inserted], + ) # Bump updated_at on the parent cluster conn.execute( "UPDATE manual_clusters SET updated_at = ? WHERE cluster_id = ?", (_utcnow(), cluster_id), ) return inserted🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/database/manual_clusters.py` around lines 207 - 231, The loop in db_add_images_to_manual_cluster is slow; instead, query existing image_ids for the given cluster_id via get_db_connection() (SELECT image_id FROM manual_cluster_images WHERE cluster_id = ? AND image_id IN (...)) to compute the delta, then use conn.executemany(...) to perform a single bulk "INSERT OR IGNORE INTO manual_cluster_images (cluster_id, image_id) VALUES (?, ?)" for only the new image ids, update the parent row's updated_at with _utcnow(), and return the list of actually-inserted ids; reference the function db_add_images_to_manual_cluster, get_db_connection, and _utcnow when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/database/manual_clusters.py`:
- Around line 23-29: The TypedDict ManualClusterRecord is missing the
image_count field which _row_to_cluster(row, include_count=True) always adds;
update ManualClusterRecord to include image_count: int (or mark it Optional[int]
if it can be missing) so the declared return types of db_get_all_manual_clusters
and db_get_manual_cluster_by_id correctly reflect the actual dict shape returned
by _row_to_cluster; ensure the type matches how _row_to_cluster populates
image_count and adjust callers only if image_count can be absent/None.
- Around line 234-249: Move the return inside the database context to avoid
reading cursor.rowcount after the connection is closed: in
db_remove_image_from_manual_cluster, keep the with get_db_connection() as conn:
block wrapping the DELETE and UPDATE logic and return cursor.rowcount > 0 before
exiting that block (i.e., directly after the optional UPDATE using cursor and
conn), referencing the existing cursor, conn, get_db_connection, _utcnow,
manual_cluster_images and manual_clusters symbols so the function returns while
the connection is still open.
In `@backend/app/routes/manual_clusters.py`:
- Around line 116-123: The catch-all except blocks in manual_clusters.py (where
logger.exception(...) is used and an HTTPException is raised with ErrorResponse)
currently put message=str(exc) into the response and leak internal details;
change each handler to keep logger.exception(exc) for server-side diagnostics
but replace message=str(exc) with a generic message such as "Internal server
error" (or "An internal error occurred") when constructing ErrorResponse passed
to HTTPException, and apply this change to all similar blocks that raise
HTTPException(status_code=500, detail=ErrorResponse(...).model_dump()) so no
exception text is returned to clients.
In `@backend/app/schemas/manual_clusters.py`:
- Around line 115-119: AssignImagesResponse currently places assigned_count and
skipped_count at the top level, breaking the common response envelope and
forcing an any cast in useManualClusters.ts; update AssignImagesResponse to wrap
the payload into a data object (e.g., data: { assigned_count: int,
skipped_count: int }) so the schema matches other responses and the frontend can
read response.data.assigned_count without casting, then update any server code
that returns this model to populate the data field and ensure the client hook
useManualClusters.ts reads the counts from response.data.
- Around line 26-45: Extract the duplicated name field and validator into a
shared BaseModel (e.g., class ClusterNameRequest(BaseModel)) that defines name:
str and the `@field_validator` "name" method name_must_not_be_blank, then have
CreateClusterRequest and RenameClusterRequest inherit from ClusterNameRequest
instead of redefining the field and validator so both classes are DRY and reuse
the common validation logic.
- Line 76: Rename the Pydantic model field thumbnailPath to snake_case
(thumbnail_path) and add a JSON alias so external JSON still uses
"thumbnailPath"; update the model by changing the attribute in the model class
(e.g., ManualCluster or whichever class contains thumbnailPath) to
thumbnail_path: Optional[str] = None and supply an alias via
Field(alias="thumbnailPath") or via model_config/Config alias settings (and
enable allow_population_by_field_name if needed) so internal Python code follows
PEP 8 while wire format remains unchanged.
In `@backend/app/utils/manual_cluster_service.py`:
- Around line 53-64: The docstring mentions DuplicateClusterNameError but the
create_cluster function never raises it; either remove the misleading docstring
entry and delete the unused DuplicateClusterNameError class, or implement a
uniqueness check and raise it: before calling db_insert_manual_cluster in
create_cluster(name: str) query for an existing cluster by name (e.g., via an
existing db lookup helper or add one), if found raise DuplicateClusterNameError,
otherwise proceed to call db_insert_manual_cluster and log as before.
- Around line 98-103: In rename_cluster, avoid the TOCTOU by capturing the
result of _require_cluster(cluster_id) into a local variable (e.g., cluster),
remove the redundant comment, call db_update_manual_cluster_name(cluster_id,
name), update cluster.name = name and return that cluster instead of calling
db_get_manual_cluster_by_id; also remove the unnecessary "# type:
ignore[return-value]" and keep the existing logger.info("Manual cluster renamed:
id=%s new_name=%s", cluster_id, name) call. This reuses the fetched value,
prevents a race where a concurrent delete makes db_get_manual_cluster_by_id
return None, and preserves type safety.
In `@backend/tests/test_manual_clusters.py`:
- Around line 56-81: Add a test method to TestCreateCluster that verifies the
POST /clusters/ returns 409 when create_cluster raises
DuplicateClusterNameError: patch "app.routes.manual_clusters.create_cluster" to
raise app.utils.manual_cluster_service.DuplicateClusterNameError (use
side_effect), call client.post("/clusters/", json={"name": "Existing"}) and
assert response.status_code == 409; name the test e.g.
test_create_cluster_duplicate_name_returns_409 and import or reference
DuplicateClusterNameError in the test.
In `@frontend/src/components/Clusters/ImagePickerModal.tsx`:
- Around line 60-66: When debouncedSearch changes we must reset the pagination
so previously loaded pages don't carry over; update the effect that sets
debouncedSearch (the useEffect using searchTimer and calling setDebouncedSearch)
to also call setPage(1) (or add a separate useEffect that watches
debouncedSearch and calls setPage(1)), ensuring the component state variable
page is reset whenever debouncedSearch changes so paged (page * PAGE_SIZE)
reflects results from the first page; apply the same reset logic to the other
search-related effect that manages debouncedSearch/filters in ImagePickerModal
so all search updates reset page.
- Around line 109-111: The selectAll callback currently sets selection to all
filtered image IDs including those in assignedIds; update selectAll so it builds
the new Set from filtered.map(i => i.id) but filters out any id present in
assignedIds before calling setSelected, and add assignedIds to the useCallback
dependency array so the callback updates when assignments change; this keeps
PickerImageCard (which blocks toggling assigned images) consistent and prevents
already-assigned images from being sent to onAssign.
In `@frontend/src/hooks/useManualClusters.ts`:
- Around line 196-221: The removeImageFromClusters function currently omits
updating the hook's loading state; update removeImageFromClusters to call
setLoading(true) at the start and setLoading(false) in a finally block so
loading is cleared on both success and failure, preserving the existing
optimistic dispatch(removeImage({ clusterId, imageId })), rollback logic that
uses fetchManualClusterById and dispatch(setClusterDetail(...)), and the boolean
return values; ensure setLoading(false) runs after both the normal return true
and all error/rollback returns.
- Around line 178-179: The cast to any is used because AssignImagesResponse
places assigned_count/skipped_count at the top level; update the typing and
access pattern to avoid any: replace uses of (res as any).assigned_count and
(res as any).skipped_count in useManualClusters (the block setting assigned and
skipped) with properly typed accesses from AssignImagesResponse (e.g.,
res.data.assigned_count / res.data.skipped_count) or update AssignImagesResponse
to include those fields in a typed data envelope, then change the variable type
from unknown/any to AssignImagesResponse so no any cast is needed.
In `@frontend/src/pages/Clusters/ClusterDetailPage.tsx`:
- Around line 273-276: The onRemove prop in ClusterImageCardProps is declared as
returning void but is awaited by ClusterImageCard and implemented by the async
function handleRemove; update the prop signature in interface
ClusterImageCardProps so onRemove returns Promise<void> (i.e., change its type
to (imageId: string) => Promise<void>), and ensure any callers of
ClusterImageCard (or implementations like handleRemove) continue to return a
Promise<void>.
---
Nitpick comments:
In `@backend/app/database/manual_clusters.py`:
- Around line 207-231: The loop in db_add_images_to_manual_cluster is slow;
instead, query existing image_ids for the given cluster_id via
get_db_connection() (SELECT image_id FROM manual_cluster_images WHERE cluster_id
= ? AND image_id IN (...)) to compute the delta, then use conn.executemany(...)
to perform a single bulk "INSERT OR IGNORE INTO manual_cluster_images
(cluster_id, image_id) VALUES (?, ?)" for only the new image ids, update the
parent row's updated_at with _utcnow(), and return the list of actually-inserted
ids; reference the function db_add_images_to_manual_cluster, get_db_connection,
and _utcnow when making the changes.
In `@backend/app/routes/manual_clusters.py`:
- Line 201: The cluster_id parameter is inconsistently annotated across
endpoints; update the signature of rename_cluster_endpoint (and similarly
assign_images_endpoint) to use FastAPI's Path(...) annotation for cluster_id so
it matches other handlers (e.g., the endpoints at lines referenced that already
use Path(...)); change the parameter declaration to accept cluster_id: str =
Path(..., description="cluster id") (or equivalent) to harmonize annotations and
include an explanatory description if present in other handlers.
In `@backend/tests/test_manual_clusters.py`:
- Around line 143-167: Add a new test method in the TestRenameCluster class
(e.g., test_rename_conflict) that patches
app.routes.manual_clusters.rename_cluster to raise DuplicateClusterNameError and
then calls the PATCH /clusters/{id} endpoint with the conflicting name; assert
the response.status_code is 409 and the body indicates conflict. Reference the
DuplicateClusterNameError exception and the rename_cluster patch used in other
tests to implement this test consistently with test_rename_missing_cluster.
In `@frontend/src/components/Clusters/ClusterCard.tsx`:
- Around line 48-50: Replace the hardcoded path in handleCardClick with the
ROUTES constant: instead of navigate(`/clusters/${cluster.cluster_id}`) use the
ROUTES.CLUSTER_DETAIL template (or a helper that formats it) to build the URL
using cluster.cluster_id; update the call in ClusterCard's handleCardClick and
mirror the same change where the literal path is used in ClusterDetailPage (the
occurrences around the handlers at lines referenced) so all navigation uses
ROUTES.CLUSTER_DETAIL with the id parameter rather than a duplicated string.
In `@frontend/src/features/manualClustersSlice.ts`:
- Around line 51-69: The appendImages reducer in manualClustersSlice.ts is
unused (no imports/dispatches) while useManualClusters.ts's assignImages calls
setClusterDetail after the API update; either remove appendImages to shrink the
slice surface or keep it but add a short TODO comment explaining its purpose
(e.g., intended for incremental client-side updates to avoid full reloads) and
reference where it would be used (assignImages / setClusterDetail) so future
devs know why it exists; update exports accordingly (remove from export list if
deleting, or leave exported with the explanatory comment if retaining).
In `@frontend/src/hooks/useManualClusters.ts`:
- Line 35: The code uses the untyped useDispatch() in useManualClusters.ts which
loses action type-safety; create a typed hook useAppDispatch that returns
useDispatch<AppDispatch>() (or add/consume an existing useAppDispatch) and
replace the raw useDispatch() call with useAppDispatch(); import AppDispatch
from your store types if needed and update the declaration `const dispatch =
useDispatch();` to `const dispatch = useAppDispatch();` so dispatched actions in
useManualClusters are properly typed.
In `@frontend/src/pages/Clusters/ClusterDetailPage.tsx`:
- Around line 42-45: The effect currently silences eslint for missing deps;
update the useEffect for loading details to include loadClusterDetail in the
dependency array (i.e., useEffect(() => { if (clusterId)
loadClusterDetail(clusterId); }, [clusterId, loadClusterDetail])) since
loadClusterDetail is already memoized via useCallback([dispatch]) and won't
cause extra renders; keep clusterId in the deps as well and remove the
eslint-disable comment.
- Around line 47-55: The code is accessing nested response data via
allImagesData?.data instead of using the hook's normalized success payload;
update the use of usePictoQuery to read from its successData field (provided by
useQueryExtension) — replace references to allImagesData and the memo that
derives allImages from allImagesData?.data with successData (e.g., const {
successData: allImagesSuccess } = usePictoQuery(...) and derive allImages from
that) while keeping the existing queryKey and queryFn (fetchAllImages) and
preserving the allImages: Image[] type and useMemo wrapper.
- Around line 290-294: Replace the inline style on the JSX container with
Tailwind's utility: in the ClusterDetailPage component update the returned <div>
(the element with className "group relative overflow-hidden rounded-md border")
to remove the style={{ aspectRatio: '1/1' }} prop and add the Tailwind class
"aspect-square" to its className so it becomes "group relative overflow-hidden
rounded-md border aspect-square".
In `@frontend/src/pages/Clusters/ClusterListPage.tsx`:
- Around line 32-35: Replace the eslint suppression by including the memoized
loadClusters function in the useEffect dependency array: update the useEffect
that currently calls loadClusters() so its deps include loadClusters, and remove
the "// eslint-disable-next-line react-hooks/exhaustive-deps" comment;
loadClusters is defined with useCallback and only depends on dispatch so it is
stable and safe to add as a dependency.
- Around line 119-126: The Dialog's onOpenChange currently closes the modal and
resets state even when a create request is in flight; update the onOpenChange
handler (the function passed to Dialog) to guard against closing when the
creating flag is true by returning early if v is false and creating is true, and
only call setShowCreate(false) and setNewName('') when not creating; reference
Dialog's onOpenChange, the creating variable, setShowCreate and setNewName to
locate where to add the guard.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
backend/app/database/manual_clusters.pybackend/app/routes/manual_clusters.pybackend/app/schemas/manual_clusters.pybackend/app/utils/manual_cluster_service.pybackend/main.pybackend/tests/conftest.pybackend/tests/test_manual_clusters.pyfrontend/src/api/api-functions/index.tsfrontend/src/api/api-functions/manual_clusters.tsfrontend/src/api/apiEndpoints.tsfrontend/src/app/store.tsfrontend/src/components/Clusters/ClusterCard.tsxfrontend/src/components/Clusters/ImagePickerModal.tsxfrontend/src/components/Navigation/Sidebar/AppSidebar.tsxfrontend/src/constants/routes.tsfrontend/src/features/manualClustersSlice.tsfrontend/src/hooks/useManualClusters.tsfrontend/src/pages/Clusters/ClusterDetailPage.tsxfrontend/src/pages/Clusters/ClusterListPage.tsxfrontend/src/routes/AppRoutes.tsxfrontend/src/types/ManualCluster.ts
Comment on lines
+23
to
+29
| class ManualClusterRecord(TypedDict): | ||
| cluster_id: str | ||
| name: str | ||
| created_at: str | ||
| updated_at: str | ||
| is_auto_generated: bool | ||
|
|
Contributor
There was a problem hiding this comment.
ManualClusterRecord TypedDict is missing image_count — return type annotations are inaccurate.
db_get_all_manual_clusters and db_get_manual_cluster_by_id both call _row_to_cluster(row, include_count=True), which unconditionally adds image_count to the returned dict. However, ManualClusterRecord has no image_count field, so the declared return types List[ManualClusterRecord] and Optional[ManualClusterRecord] are incorrect. Callers relying on these type hints will get no IDE/mypy support for image_count.
♻️ Proposed fix
class ManualClusterRecord(TypedDict):
cluster_id: str
name: str
created_at: str
updated_at: str
is_auto_generated: bool
+ image_count: intAlso applies to: 101-121, 124-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/database/manual_clusters.py` around lines 23 - 29, The TypedDict
ManualClusterRecord is missing the image_count field which _row_to_cluster(row,
include_count=True) always adds; update ManualClusterRecord to include
image_count: int (or mark it Optional[int] if it can be missing) so the declared
return types of db_get_all_manual_clusters and db_get_manual_cluster_by_id
correctly reflect the actual dict shape returned by _row_to_cluster; ensure the
type matches how _row_to_cluster populates image_count and adjust callers only
if image_count can be absent/None.
Comment on lines
+234
to
+249
| def db_remove_image_from_manual_cluster(cluster_id: str, image_id: str) -> bool: | ||
| """Remove a single image from a cluster. Returns True if removed.""" | ||
| with get_db_connection() as conn: | ||
| cursor = conn.execute( | ||
| """ | ||
| DELETE FROM manual_cluster_images | ||
| WHERE cluster_id = ? AND image_id = ? | ||
| """, | ||
| (cluster_id, image_id), | ||
| ) | ||
| if cursor.rowcount > 0: | ||
| conn.execute( | ||
| "UPDATE manual_clusters SET updated_at = ? WHERE cluster_id = ?", | ||
| (_utcnow(), cluster_id), | ||
| ) | ||
| return cursor.rowcount > 0 |
Contributor
There was a problem hiding this comment.
cursor.rowcount is accessed on line 249 outside the with block — move the return inside.
The with get_db_connection() as conn: block closes (and commits) the connection on exit. While the cursor local variable remains in scope and its rowcount attribute is still readable after the with exits, this is fragile and non-obvious. Move the return inside the block for clarity and safety.
♻️ Proposed fix
def db_remove_image_from_manual_cluster(cluster_id: str, image_id: str) -> bool:
"""Remove a single image from a cluster. Returns True if removed."""
with get_db_connection() as conn:
cursor = conn.execute(
"""
DELETE FROM manual_cluster_images
WHERE cluster_id = ? AND image_id = ?
""",
(cluster_id, image_id),
)
if cursor.rowcount > 0:
conn.execute(
"UPDATE manual_clusters SET updated_at = ? WHERE cluster_id = ?",
(_utcnow(), cluster_id),
)
- return cursor.rowcount > 0
+ return cursor.rowcount > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/database/manual_clusters.py` around lines 234 - 249, Move the
return inside the database context to avoid reading cursor.rowcount after the
connection is closed: in db_remove_image_from_manual_cluster, keep the with
get_db_connection() as conn: block wrapping the DELETE and UPDATE logic and
return cursor.rowcount > 0 before exiting that block (i.e., directly after the
optional UPDATE using cursor and conn), referencing the existing cursor, conn,
get_db_connection, _utcnow, manual_cluster_images and manual_clusters symbols so
the function returns while the connection is still open.
Comment on lines
+116
to
+123
| except Exception as exc: | ||
| logger.exception("Unexpected error creating cluster") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=ErrorResponse( | ||
| success=False, error="Internal Error", message=str(exc) | ||
| ).model_dump(), | ||
| ) |
Contributor
There was a problem hiding this comment.
Internal error details are leaked to the client in 500 responses.
All catch-all handlers use message=str(exc), which may expose stack traces, file paths, or database details to the API consumer. Use a generic message for the response and rely on the logger.exception call for server-side diagnostics.
🛡️ Suggested change (apply to all 500 handlers)
- detail=ErrorResponse(
- success=False, error="Internal Error", message=str(exc)
- ).model_dump(),
+ detail=ErrorResponse(
+ success=False, error="Internal Error", message="An unexpected error occurred."
+ ).model_dump(),Also applies to: 145-152, 180-187, 214-221, 245-252, 289-296, 329-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/routes/manual_clusters.py` around lines 116 - 123, The catch-all
except blocks in manual_clusters.py (where logger.exception(...) is used and an
HTTPException is raised with ErrorResponse) currently put message=str(exc) into
the response and leak internal details; change each handler to keep
logger.exception(exc) for server-side diagnostics but replace message=str(exc)
with a generic message such as "Internal server error" (or "An internal error
occurred") when constructing ErrorResponse passed to HTTPException, and apply
this change to all similar blocks that raise HTTPException(status_code=500,
detail=ErrorResponse(...).model_dump()) so no exception text is returned to
clients.
Comment on lines
+26
to
+45
| class CreateClusterRequest(BaseModel): | ||
| name: str | ||
|
|
||
| @field_validator("name") | ||
| @classmethod | ||
| def name_must_not_be_blank(cls, v: str) -> str: | ||
| if not v.strip(): | ||
| raise ValueError("Cluster name must not be blank") | ||
| return v.strip() | ||
|
|
||
|
|
||
| class RenameClusterRequest(BaseModel): | ||
| name: str | ||
|
|
||
| @field_validator("name") | ||
| @classmethod | ||
| def name_must_not_be_blank(cls, v: str) -> str: | ||
| if not v.strip(): | ||
| raise ValueError("Cluster name must not be blank") | ||
| return v.strip() |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
CreateClusterRequest and RenameClusterRequest are identical — extract a shared base.
Both classes have the same name: str field and the same name_must_not_be_blank validator. This is a direct DRY violation.
♻️ Proposed refactor
+class _ClusterNameMixin(BaseModel):
+ name: str
+
+ `@field_validator`("name")
+ `@classmethod`
+ def name_must_not_be_blank(cls, v: str) -> str:
+ if not v.strip():
+ raise ValueError("Cluster name must not be blank")
+ return v.strip()
+
+
-class CreateClusterRequest(BaseModel):
- name: str
-
- `@field_validator`("name")
- `@classmethod`
- def name_must_not_be_blank(cls, v: str) -> str:
- if not v.strip():
- raise ValueError("Cluster name must not be blank")
- return v.strip()
-
-
-class RenameClusterRequest(BaseModel):
- name: str
-
- `@field_validator`("name")
- `@classmethod`
- def name_must_not_be_blank(cls, v: str) -> str:
- if not v.strip():
- raise ValueError("Cluster name must not be blank")
- return v.strip()
+class CreateClusterRequest(_ClusterNameMixin):
+ pass
+
+
+class RenameClusterRequest(_ClusterNameMixin):
+ pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/schemas/manual_clusters.py` around lines 26 - 45, Extract the
duplicated name field and validator into a shared BaseModel (e.g., class
ClusterNameRequest(BaseModel)) that defines name: str and the `@field_validator`
"name" method name_must_not_be_blank, then have CreateClusterRequest and
RenameClusterRequest inherit from ClusterNameRequest instead of redefining the
field and validator so both classes are DRY and reuse the common validation
logic.
| class ImageInCluster(BaseModel): | ||
| id: str | ||
| path: str | ||
| thumbnailPath: Optional[str] = None |
Contributor
There was a problem hiding this comment.
thumbnailPath is camelCase — use snake_case with a field alias for JSON serialization.
Python field names must be snake_case per PEP 8. Use Pydantic's alias or model_config to keep the JSON wire format unchanged.
♻️ Proposed fix
class ImageInCluster(BaseModel):
+ model_config = ConfigDict(populate_by_name=True)
+
id: str
path: str
- thumbnailPath: Optional[str] = None
+ thumbnail_path: Optional[str] = Field(None, alias="thumbnailPath")
metadata: Optional[Any] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/schemas/manual_clusters.py` at line 76, Rename the Pydantic model
field thumbnailPath to snake_case (thumbnail_path) and add a JSON alias so
external JSON still uses "thumbnailPath"; update the model by changing the
attribute in the model class (e.g., ManualCluster or whichever class contains
thumbnailPath) to thumbnail_path: Optional[str] = None and supply an alias via
Field(alias="thumbnailPath") or via model_config/Config alias settings (and
enable allow_population_by_field_name if needed) so internal Python code follows
PEP 8 while wire format remains unchanged.
Comment on lines
+60
to
+66
| useEffect(() => { | ||
| if (searchTimer.current) clearTimeout(searchTimer.current); | ||
| searchTimer.current = setTimeout(() => setDebouncedSearch(search), 300); | ||
| return () => { | ||
| if (searchTimer.current) clearTimeout(searchTimer.current); | ||
| }; | ||
| }, [search]); |
Contributor
There was a problem hiding this comment.
Page count doesn't reset when search query changes.
If the user has loaded several pages (e.g., page=5) and then types a search query, paged will show up to page * PAGE_SIZE items from the new filtered results rather than resetting to the first page. Consider resetting page when debouncedSearch changes.
♻️ Proposed fix: reset page on search
// Debounce search input
useEffect(() => {
if (searchTimer.current) clearTimeout(searchTimer.current);
- searchTimer.current = setTimeout(() => setDebouncedSearch(search), 300);
+ searchTimer.current = setTimeout(() => {
+ setDebouncedSearch(search);
+ setPage(1);
+ }, 300);
return () => {
if (searchTimer.current) clearTimeout(searchTimer.current);
};
}, [search]);Also applies to: 78-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/Clusters/ImagePickerModal.tsx` around lines 60 - 66,
When debouncedSearch changes we must reset the pagination so previously loaded
pages don't carry over; update the effect that sets debouncedSearch (the
useEffect using searchTimer and calling setDebouncedSearch) to also call
setPage(1) (or add a separate useEffect that watches debouncedSearch and calls
setPage(1)), ensuring the component state variable page is reset whenever
debouncedSearch changes so paged (page * PAGE_SIZE) reflects results from the
first page; apply the same reset logic to the other search-related effect that
manages debouncedSearch/filters in ImagePickerModal so all search updates reset
page.
Comment on lines
+109
to
+111
| const selectAll = useCallback(() => { | ||
| setSelected(new Set(filtered.map((i) => i.id))); | ||
| }, [filtered]); |
Contributor
There was a problem hiding this comment.
selectAll includes already-assigned images, causing incorrect behavior.
selectAll adds every filtered image ID to the selection, including those in assignedIds. Since PickerImageCard prevents toggling assigned images off (line 237), the user cannot deselect them, and they will be sent to onAssign — potentially causing duplicate-assignment errors or confusing UI state.
🐛 Proposed fix: exclude already-assigned images
const selectAll = useCallback(() => {
- setSelected(new Set(filtered.map((i) => i.id)));
+ setSelected(new Set(filtered.filter((i) => !assignedIds.has(i.id)).map((i) => i.id)));
- }, [filtered]);
+ }, [filtered, assignedIds]);📝 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.
Suggested change
| const selectAll = useCallback(() => { | |
| setSelected(new Set(filtered.map((i) => i.id))); | |
| }, [filtered]); | |
| const selectAll = useCallback(() => { | |
| setSelected(new Set(filtered.filter((i) => !assignedIds.has(i.id)).map((i) => i.id))); | |
| }, [filtered, assignedIds]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/Clusters/ImagePickerModal.tsx` around lines 109 -
111, The selectAll callback currently sets selection to all filtered image IDs
including those in assignedIds; update selectAll so it builds the new Set from
filtered.map(i => i.id) but filters out any id present in assignedIds before
calling setSelected, and add assignedIds to the useCallback dependency array so
the callback updates when assignments change; this keeps PickerImageCard (which
blocks toggling assigned images) consistent and prevents already-assigned images
from being sent to onAssign.
Comment on lines
+178
to
+179
| assigned: (res as any).assigned_count ?? 0, | ||
| skipped: (res as any).skipped_count ?? 0, |
Contributor
There was a problem hiding this comment.
(res as any) violates the no-any guideline — root cause is the schema design.
The any cast exists because AssignImagesResponse places assigned_count/skipped_count at the top level of the response rather than inside a typed data field (see schema comment above). Once the backend schema is aligned with the envelope pattern, this can be typed properly:
- return {
- assigned: (res as any).assigned_count ?? 0,
- skipped: (res as any).skipped_count ?? 0,
- };
+ return {
+ assigned: res.data?.assigned_count ?? 0,
+ skipped: res.data?.skipped_count ?? 0,
+ };As per coding guidelines, any must be avoided; use explicit types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/useManualClusters.ts` around lines 178 - 179, The cast to
any is used because AssignImagesResponse places assigned_count/skipped_count at
the top level; update the typing and access pattern to avoid any: replace uses
of (res as any).assigned_count and (res as any).skipped_count in
useManualClusters (the block setting assigned and skipped) with properly typed
accesses from AssignImagesResponse (e.g., res.data.assigned_count /
res.data.skipped_count) or update AssignImagesResponse to include those fields
in a typed data envelope, then change the variable type from unknown/any to
AssignImagesResponse so no any cast is needed.
Comment on lines
+196
to
+221
| const removeImageFromClusters = useCallback( | ||
| async (clusterId: string, imageId: string): Promise<boolean> => { | ||
| // Optimistic update | ||
| dispatch(removeImage({ clusterId, imageId })); | ||
| try { | ||
| const res = await removeImageFromCluster(clusterId, imageId); | ||
| if (!res.success) { | ||
| // Rollback: reload detail | ||
| const detailRes = await fetchManualClusterById(clusterId); | ||
| if (detailRes.success && detailRes.data) { | ||
| dispatch(setClusterDetail(detailRes.data as ManualClusterDetail)); | ||
| } | ||
| setError(res.error ?? 'Failed to remove image'); | ||
| return false; | ||
| } | ||
| return true; | ||
| } catch { | ||
| // Rollback | ||
| const detailRes = await fetchManualClusterById(clusterId); | ||
| if (detailRes.success && detailRes.data) { | ||
| dispatch(setClusterDetail(detailRes.data as ManualClusterDetail)); | ||
| } | ||
| setError('Failed to remove image'); | ||
| return false; | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
removeImageFromClusters never sets or clears loading — inconsistent with all other operations.
Every other exported function in this hook calls setLoading(true) at entry and setLoading(false) in a finally block. removeImageFromClusters skips this entirely, so consumers cannot rely on the loading flag to show a spinner or disable UI while the delete is in flight.
♻️ Proposed fix
const removeImageFromClusters = useCallback(
async (clusterId: string, imageId: string): Promise<boolean> => {
+ setLoading(true);
+ setError(null);
// Optimistic update
dispatch(removeImage({ clusterId, imageId }));
try {
const res = await removeImageFromCluster(clusterId, imageId);
if (!res.success) {
// Rollback: reload detail
const detailRes = await fetchManualClusterById(clusterId);
if (detailRes.success && detailRes.data) {
dispatch(setClusterDetail(detailRes.data as ManualClusterDetail));
}
setError(res.error ?? 'Failed to remove image');
return false;
}
return true;
} catch {
// Rollback
const detailRes = await fetchManualClusterById(clusterId);
if (detailRes.success && detailRes.data) {
dispatch(setClusterDetail(detailRes.data as ManualClusterDetail));
}
setError('Failed to remove image');
return false;
+ } finally {
+ setLoading(false);
}
},
[dispatch],
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/useManualClusters.ts` around lines 196 - 221, The
removeImageFromClusters function currently omits updating the hook's loading
state; update removeImageFromClusters to call setLoading(true) at the start and
setLoading(false) in a finally block so loading is cleared on both success and
failure, preserving the existing optimistic dispatch(removeImage({ clusterId,
imageId })), rollback logic that uses fetchManualClusterById and
dispatch(setClusterDetail(...)), and the boolean return values; ensure
setLoading(false) runs after both the normal return true and all error/rollback
returns.
Comment on lines
+273
to
+276
| interface ClusterImageCardProps { | ||
| image: ManualClusterImage; | ||
| onRemove: (imageId: string) => void; | ||
| } |
Contributor
There was a problem hiding this comment.
onRemove return type should be Promise<void>, not void.
handleRemove (line 108) returns Promise<void>, and ClusterImageCard awaits it (line 286). Declaring the prop as void hides the async nature and can mislead callers/linters.
♻️ Suggested fix
interface ClusterImageCardProps {
image: ManualClusterImage;
- onRemove: (imageId: string) => void;
+ onRemove: (imageId: string) => Promise<void>;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/Clusters/ClusterDetailPage.tsx` around lines 273 - 276,
The onRemove prop in ClusterImageCardProps is declared as returning void but is
awaited by ClusterImageCard and implemented by the async function handleRemove;
update the prop signature in interface ClusterImageCardProps so onRemove returns
Promise<void> (i.e., change its type to (imageId: string) => Promise<void>), and
ensure any callers of ClusterImageCard (or implementations like handleRemove)
continue to return a Promise<void>.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backend:
Frontend:
Addressed Issues:
Fixes #(TODO:issue number)
Screenshots/Recordings:
Additional Notes:
Checklist
AI Usage Disclosure
Check one of the checkboxes below:
Summary by CodeRabbit
Release Notes
New Features