Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPreserves HTTPExceptions in the toggle-favourite route, returns structured Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as ImagesRoute
participant DB as ImagesDB
participant Utils as MetadataUtil
Client->>API: POST /images/toggle-favourite { image_id }
API->>DB: db_toggle_image_favourite_status(image_id)
alt toggle returned None (not found / failed)
DB-->>API: None
API-->>Client: 404 ErrorResponse ("Image not found or failed to toggle")
else toggle succeeded (bool)
DB-->>API: True/False
API->>DB: db_get_image_by_id(image_id)
DB-->>API: image_row or None
alt image_row found
API->>Utils: image_util_parse_metadata(image_row.metadata)
Utils-->>API: merged metadata dict
API-->>Client: 200 { image with updated isFavourite }
else image_row missing
API-->>Client: 404 ErrorResponse ("Image not found after toggling")
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
![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: 4
🧹 Nitpick comments (1)
backend/app/routes/images.py (1)
105-110: Extract duplicated 404 error payload construction.The same
ErrorResponse(...).model_dump()block is repeated in two places. Consider a small helper (or constant factory) to keep error shape/messages consistent and easier to maintain.
As per coding guidelines, "Look for code duplication".Also applies to: 118-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/routes/images.py` around lines 105 - 110, Create a small helper to produce the 404 error payload and replace the duplicated ErrorResponse(...).model_dump() usages: add a function like build_image_not_found_error() that returns ErrorResponse(success=False, error="Image Not Found", message="Image not found or failed to toggle").model_dump(), then call this helper from the two places currently constructing that payload (the HTTPException raises in routes/images.py around the toggling logic). Update both occurrences to use build_image_not_found_error() so the shape/messages are consistent and duplication is removed.
🤖 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/routes/images.py`:
- Around line 116-124: The current post-toggle read maps any failure to a 404 by
checking `image is None` after calling the generic read; instead call a DB
method that clearly distinguishes “not found” from internal errors (e.g., use or
add a `db_get_image_by_id(image_id)` or similar in
`backend/app/database/images.py`) and propagate DB exceptions rather than
treating them as empty results; change the logic around the `image` check in the
toggle handler to catch and re-raise DB errors (500) while only returning the
HTTP 404 when the DB call returns a definitive not-found result (None),
referencing the toggle handler's `image` variable and the new/updated
`db_get_image_by_id` function to locate the change.
In `@backend/app/utils/images.py`:
- Around line 553-562: The code parses db_metadata with json.loads and then
merges it into defaults, but json.loads can return non-dict values
(list/str/number) causing a TypeError when performing {**defaults, **parsed};
modify the parsing branch around db_metadata so that after json.loads you verify
parsed is a dict (e.g., isinstance(parsed, dict)) and if not, set parsed = {}
(or coerce only expected keys), ensuring the merge {**defaults, **parsed} only
receives a mapping; references: the db_metadata variable and the return merge
expression that currently does "return {**defaults, **parsed}"—add the dict-type
guard immediately after json.loads and before the merge.
In `@backend/tests/test_toggle_favourite.py`:
- Around line 24-31: The test test_get_all_images_crash currently only checks
for a 200 and success flag; update it to also validate that the first returned
image includes required metadata keys by inspecting response.json() (use the
existing response variable) and asserting that the first image's metadata
contains "name", "width", "height", "file_location", and "item_type" (and
optionally their default non-null/expected default values) so the contract about
metadata defaults is enforced.
- Around line 9-23: The dummy_image fixture inserts a test row into the images
table but never removes it; change the fixture to perform teardown by yielding
the image_id (use the fixture named dummy_image) and after the yield delete the
inserted row (DELETE FROM images WHERE id = ?) using the same DATABASE_PATH
connection and commit/close the connection to ensure no persistent DB state
leaks between tests; ensure the fixture still returns the image_id to callers
and that cleanup runs even if tests fail.
---
Nitpick comments:
In `@backend/app/routes/images.py`:
- Around line 105-110: Create a small helper to produce the 404 error payload
and replace the duplicated ErrorResponse(...).model_dump() usages: add a
function like build_image_not_found_error() that returns
ErrorResponse(success=False, error="Image Not Found", message="Image not found
or failed to toggle").model_dump(), then call this helper from the two places
currently constructing that payload (the HTTPException raises in
routes/images.py around the toggling logic). Update both occurrences to use
build_image_not_found_error() so the shape/messages are consistent and
duplication is removed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/routes/images.pybackend/app/utils/images.pybackend/tests/test_toggle_favourite.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/images.py`:
- Line 291: The mapping for "folder_id" treats 0 as falsey; update the
conditional to explicitly check for None so a folder_id of 0 is
preserved—replace the check in the mapping (the line that currently reads
"folder_id": str(row[2]) if row[2] else None) with an explicit None check (e.g.,
"folder_id": str(row[2]) if row[2] is not None else None) so row[2]==0 is
handled correctly.
In `@backend/app/routes/images.py`:
- Around line 131-133: The catch block in the /toggle-favourite route currently
returns the raw exception message in the HTTPException detail and leaks
internals; instead, keep the logger.error call but include exception context
(e.g., pass exc_info=True or log the stack) and change the HTTPException to
return a generic, structured error (matching other handlers' format) such as a
fixed message like "Internal server error" or {"detail":"Internal server error"}
without including {e}; update the block around the logger.error and
HTTPException instantiation to only log the exception details and return the
sanitized generic response.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/routes/images.py (1)
133-140: UseErrorResponsein the 500 branch for consistency.This block already returns a safe message; consider serializing with
ErrorResponse(...).model_dump()like other branches to keep one error shape source.Consistency-only refactor
- raise HTTPException( - status_code=500, - detail={ - "success": False, - "error": "Internal Server Error", - "message": "An unexpected error occurred while toggling favourite", - }, - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=ErrorResponse( + success=False, + error="Internal Server Error", + message="An unexpected error occurred while toggling favourite", + ).model_dump(), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/routes/images.py` around lines 133 - 140, Replace the raw dict used in the HTTPException for the 500 branch with a serialized ErrorResponse to match other error branches: construct ErrorResponse(success=False, error="Internal Server Error", message="An unexpected error occurred while toggling favourite") and pass ErrorResponse(...).model_dump() as the detail when raising HTTPException(status_code=500, ...); keep the same status code and message text but use ErrorResponse.model_dump() for consistency with other error responses.
🤖 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/routes/images.py`:
- Around line 102-111: db_toggle_image_favourite_status currently conflates "not
found" and internal DB errors by returning False for both; update the route
handler to distinguish outcomes: call db_toggle_image_favourite_status(image_id)
and treat three cases — a not-found sentinel (e.g., None or a specific enum)
should raise HTTP 404 with "Image Not Found", a DB failure (False or an
exception) should raise HTTP 500 with an internal error message, and success
should continue as normal; reference db_toggle_image_favourite_status
(backend/app/database/images.py) and the route in images.py to implement the
explicit branching and convert DB exceptions to 500 (or propagate) instead of
mapping them to 404.
---
Nitpick comments:
In `@backend/app/routes/images.py`:
- Around line 133-140: Replace the raw dict used in the HTTPException for the
500 branch with a serialized ErrorResponse to match other error branches:
construct ErrorResponse(success=False, error="Internal Server Error",
message="An unexpected error occurred while toggling favourite") and pass
ErrorResponse(...).model_dump() as the detail when raising
HTTPException(status_code=500, ...); keep the same status code and message text
but use ErrorResponse.model_dump() for consistency with other error responses.
backend/app/routes/images.py
Outdated
Comment on lines
102
to
111
| success = db_toggle_image_favourite_status(image_id) | ||
| if not success: | ||
| raise HTTPException( | ||
| status_code=404, detail="Image not found or failed to toggle" | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail=ErrorResponse( | ||
| success=False, | ||
| error="Image Not Found", | ||
| message="Image not found or failed to toggle", | ||
| ).model_dump(), | ||
| ) |
Contributor
There was a problem hiding this comment.
Don’t map DB exceptions to 404 in this branch.
At Line 103, not success currently returns 404, but db_toggle_image_favourite_status also returns False on internal DB exceptions (backend/app/database/images.py Line 504-Line 507). That can still mislabel server failures as “Image Not Found.”
As per coding guidelines, "Confirm that the code meets the project's requirements and objectives."
Suggested fix (separate not-found from DB failure)
# backend/app/database/images.py
-def db_toggle_image_favourite_status(image_id: str) -> bool:
+def db_toggle_image_favourite_status(image_id: str) -> Optional[bool]:
@@
- if not cursor.fetchone():
- return False
+ if not cursor.fetchone():
+ return None
@@
- except Exception as e:
- logger.error(f"Database error: {e}")
- conn.rollback()
- return False
+ except Exception:
+ logger.exception("Database error while toggling favourite")
+ conn.rollback()
+ raise# backend/app/routes/images.py
- success = db_toggle_image_favourite_status(image_id)
- if not success:
+ result = db_toggle_image_favourite_status(image_id)
+ if result is None:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail=ErrorResponse(
success=False,
error="Image Not Found",
message="Image not found or failed to toggle",
).model_dump(),
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/routes/images.py` around lines 102 - 111,
db_toggle_image_favourite_status currently conflates "not found" and internal DB
errors by returning False for both; update the route handler to distinguish
outcomes: call db_toggle_image_favourite_status(image_id) and treat three cases
— a not-found sentinel (e.g., None or a specific enum) should raise HTTP 404
with "Image Not Found", a DB failure (False or an exception) should raise HTTP
500 with an internal error message, and success should continue as normal;
reference db_toggle_image_favourite_status (backend/app/database/images.py) and
the route in images.py to implement the explicit branching and convert DB
exceptions to 500 (or propagate) instead of mapping them to 404.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/database/images.py (1)
487-489: Use_connect()helper instead of directsqlite3.connect()for consistency.This function uses
sqlite3.connect(DATABASE_PATH)directly, while all other functions in the file use the_connect()helper. The helper also enables foreign key constraints (PRAGMA foreign_keys = ON), so bypassing it introduces inconsistency.♻️ Proposed fix
def db_toggle_image_favourite_status(image_id: str) -> Optional[bool]: - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/database/images.py` around lines 487 - 489, The function db_toggle_image_favourite_status uses sqlite3.connect(DATABASE_PATH) directly; replace that call with the module's _connect() helper and use the returned connection/cursor (as other DB functions do) so foreign key PRAGMA and consistent connection handling are preserved; update any variable assignments (conn, cursor) and ensure the rest of db_toggle_image_favourite_status uses those objects and closes/commits the connection unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/database/images.py`:
- Around line 487-489: The function db_toggle_image_favourite_status uses
sqlite3.connect(DATABASE_PATH) directly; replace that call with the module's
_connect() helper and use the returned connection/cursor (as other DB functions
do) so foreign key PRAGMA and consistent connection handling are preserved;
update any variable assignments (conn, cursor) and ensure the rest of
db_toggle_image_favourite_status uses those objects and closes/commits the
connection unchanged.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2e98d0d-5b5d-45be-a9df-ab94e2ccbfa3
📒 Files selected for processing (2)
backend/app/database/images.pybackend/app/routes/images.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/routes/images.py
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
2 participants
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.
Addressed Issues:
BUG: /images/toggle-favourite returns 500 instead of 404 for unknown image_id
Fixes #1179
Screenshots/Recordings:
Additional Notes:
This PR fixes issue #1179, where the /images/toggle-favourite endpoint was incorrectly returning a 500 Internal Server Error instead of a 404 Not Found when an invalid image_id was provided now it returns a 404 when an invalid image_id is provided.
Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: i have used gemini cli for test generation
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests