feat: search photo's caption #21
Conversation
WalkthroughThe changes in this pull request involve significant updates to the Changes
Possibly related issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
lib/save_it/typesense_photo.ex (3)
2-2: Consider adding logging statements for critical operations.While requiring Logger is a good practice, there are no actual logging statements in the code. Consider adding logging for critical operations like photo creation, updates, and searches to aid in monitoring and debugging.
Example additions:
def create_photo!( %{ belongs_to_id: belongs_to_id } = photo_params ) do + Logger.debug("Creating photo with params: #{inspect(photo_params)}") photo_create_input = photo_params |> Map.put(:belongs_to_id, Integer.to_string(belongs_to_id)) |> Map.put(:inserted_at, DateTime.utc_now() |> DateTime.to_unix()) Typesense.create_document!( "photos", photo_create_input ) end
Line range hint
29-41: Consider making the distance_threshold configurable.The search function uses a hardcoded distance_threshold of 0.75, which is significantly higher than the 0.4 used in
search_similar_photos!/2. This could lead to false positives in search results.Consider making it configurable similar to
search_similar_photos!/2:- def search_photos!(q) do + def search_photos!(q, opts \\ []) do + distance_threshold = Keyword.get(opts, :distance_threshold, 0.75) req_body = %{ "searches" => [ %{ "query_by" => "image_embedding,caption", "q" => q, "collection" => "photos", "prefix" => false, - "vector_query" => "image_embedding:([], k: 5, distance_threshold: 0.75)", + "vector_query" => "image_embedding:([], k: 5, distance_threshold: #{distance_threshold})", "exclude_fields" => "image_embedding" } ] }
33-33: Document the search behavior with multiple fields.The addition of "caption" to query_by improves search flexibility, but the behavior of searching across multiple fields should be documented. Does the order of fields affect search priority? Are results from both fields combined or prioritized?
Consider adding a module or function documentation:
@doc """ Searches photos using both image embeddings and captions. Results are matched against either the image embedding vector or the caption text. The search prioritizes matches in the order specified: image_embedding first, then caption. ## Parameters - q: The search query string """lib/save_it/bot.ex (4)
26-28: Consider enhancing command descriptions for better user guidance.The command descriptions could be more specific about their functionality:
- command("similar", description: "Find similar photos") - command("search", description: "Search photos") + command("similar", description: "Find visually similar photos by uploading an image") + command("search", description: "Search photos by text description")
118-130: Enhance the empty search prompt with examples.The current prompt could be more helpful by providing examples of successful search terms.
- send_message(chat.id, "What do you want to search? animal, food, etc.") + send_message(chat.id, "Please enter what you want to search for. Examples:\n- cat\n- sunset\n- beach vacation")
137-139: Remove unreachable commented code.This commented code block is noted as unreachable and should be removed to maintain code cleanliness.
321-332: Add documentation for distance threshold values.The distance threshold values (0.1 for same photos, 0.4 for similar photos) should be documented to explain their significance and how they were determined.
+ @doc """ + Finds exactly matching photos using a strict distance threshold of 0.1. + This threshold was determined through testing to minimize false positives. + """ defp find_same_photos(photo_id) do TypesensePhoto.search_similar_photos!( photo_id, distance_threshold: 0.1 ) end + @doc """ + Finds visually similar photos using a relaxed distance threshold of 0.4. + This threshold allows for finding photos with similar visual elements while + maintaining reasonable precision. + """ defp search_similar_photos(photo_id) do TypesensePhoto.search_similar_photos!( photo_id, distance_threshold: 0.4 ) end
lib/save_it/bot.ex
Outdated
| # caption: contains /similar or /search -> search similar photos; otherwise, find same photos | ||
| def handle({:message, %{chat: chat, caption: caption, photo: photos}}, ctx) do | ||
| photo = List.last(photos) | ||
|
|
||
| search_similar_photos_based_on_caption(photo, caption, | ||
| chat_id: chat.id, | ||
| bot_id: ctx.bot_info.id | ||
| ) | ||
| file = ExGram.get_file!(photo.file_id) | ||
| photo_file_content = Telegram.download_file_content!(file.file_path) | ||
|
|
||
| bot_id = ctx.bot_info.id | ||
| chat_id = chat.id | ||
|
|
||
| caption = | ||
| if String.contains?(caption, ["/similar", "/search"]) do | ||
| "" | ||
| else | ||
| caption | ||
| end | ||
|
|
||
| typesense_photo = | ||
| TypesensePhoto.create_photo!(%{ | ||
| image: Base.encode64(photo_file_content), | ||
| caption: caption, | ||
| url: photo_url(bot_id, file.file_id), | ||
| belongs_to_id: chat_id | ||
| }) | ||
|
|
||
| case caption do | ||
| "" -> | ||
| photos = search_similar_photos(typesense_photo["id"]) | ||
| answer_photos(chat.id, photos) | ||
|
|
||
| _ -> | ||
| photos = find_same_photos(typesense_photo["id"]) | ||
|
|
||
| case photos do | ||
| [] -> nil | ||
| _ -> answer_photos(chat.id, photos) | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce code duplication and simplify caption processing.
The photo processing logic is duplicated between this handler and the no-caption handler. Consider extracting common functionality into a private function.
Additionally, the caption processing could be simplified using pattern matching.
# Extract common photo processing logic
defp process_photo(photo, bot_id, chat_id, caption \\ "") do
with {:ok, file} <- ExGram.get_file(photo.file_id),
{:ok, content} <- Telegram.download_file_content(file.file_path) do
TypesensePhoto.create_photo!(%{
image: Base.encode64(content),
caption: caption,
url: photo_url(bot_id, file.file_id),
belongs_to_id: chat_id
})
end
end
# Simplify caption processing using pattern matching
def handle({:message, %{chat: chat, caption: caption, photo: [_ | _] = photos} = msg}, ctx) do
photo = List.last(photos)
case process_photo(photo, ctx.bot_info.id, chat.id, caption) do
{:ok, typesense_photo} ->
case caption do
"/similar" <> _ -> search_and_answer_similar(typesense_photo, chat.id)
"/search" <> _ -> search_and_answer_similar(typesense_photo, chat.id)
nil -> search_and_answer_same(typesense_photo, chat.id)
_ -> search_and_answer_same(typesense_photo, chat.id)
end
{:error, reason} ->
Logger.error("Failed to process photo: #{inspect(reason)}")
send_message(chat.id, "Sorry, I couldn't process your photo. Please try again.")
end
end
lib/save_it/bot.ex
Outdated
| # caption: nil -> find same photos | ||
| def handle({:message, %{chat: chat, caption: nil, photo: photos}}, ctx) do | ||
| photo = List.last(photos) | ||
|
|
||
| file = ExGram.get_file!(photo.file_id) | ||
| photo_file_content = Telegram.download_file_content!(file.file_path) | ||
|
|
||
| bot_id = ctx.bot_info.id | ||
| chat_id = chat.id | ||
|
|
||
| typesense_photo = | ||
| TypesensePhoto.create_photo!(%{ | ||
| image: Base.encode64(photo_file_content), | ||
| caption: "", | ||
| url: photo_url(bot_id, file.file_id), | ||
| belongs_to_id: chat_id | ||
| }) | ||
|
|
||
| photos = find_same_photos(typesense_photo["id"]) | ||
|
|
||
| case photos do | ||
| [] -> nil | ||
| _ -> answer_photos(chat.id, photos) | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for photo processing.
The photo processing chain (file retrieval → content download → TypesensePhoto creation) could benefit from explicit error handling to provide better user feedback when operations fail.
- file = ExGram.get_file!(photo.file_id)
- photo_file_content = Telegram.download_file_content!(file.file_path)
+ with {:ok, file} <- ExGram.get_file(photo.file_id),
+ {:ok, photo_file_content} <- Telegram.download_file_content(file.file_path) do
+ # ... rest of the processing ...
+ else
+ {:error, reason} ->
+ Logger.error("Failed to process photo: #{inspect(reason)}")
+ send_message(chat.id, "Sorry, I couldn't process your photo. Please try again.")
+ endCommittable suggestion was skipped due to low confidence.
style: 💄 clean code
ThaddeusJiang
force-pushed
the
feat/keep-silence
branch
from
77538fe to
3258ad7
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Refactor