feat: /delete command for clear messages | resolve #25 #31
Conversation
WalkthroughThe pull request introduces several changes across multiple modules in the Changes
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 (
|
ThaddeusJiang
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
lib/save_it/migration/typesense.ex (1)
Line range hint
1-53: Consider consistent error handling across all collection operations.The module now has inconsistent error handling approaches - some functions use bang methods while others don't. This could lead to confusion about error handling expectations.
Consider applying the same non-bang pattern to other functions (
create_collection!,update_collection!,list_collections!) for consistency. This would provide a uniform error handling interface across the module.lib/small_sdk/web_downloader.ex (2)
13-16: Improved error handling implementation!The new case statement properly handles both success and error scenarios from the reduction, following Elixir conventions. This makes error propagation more explicit and maintains consistent return types.
Consider adding error logging before returning the error:
case res do - {:error, reason} -> {:error, reason} + {:error, reason} -> + Logger.error("Failed to download files: #{inspect(reason)}") + {:error, reason} files -> {:ok, files} end
Line range hint
4-16: Consider restoring logging for operational visibility.While the error handling improvements are excellent, the removal of logging at the start of
download_files/1reduces operational visibility. This could make it harder to track and debug batch download operations in production.Consider adding back logging with additional context:
def download_files(urls) do + Logger.info("Starting batch download of #{length(urls)} files") res = urls |> Enum.map(&download_file/1)lib/save_it/bot.ex (2)
49-53: Consider moving GitHub URL to configuration.The GitHub URL could be moved to a configuration value for better maintainability.
- Give a star ⭐ if you like it, https://github.com/ThaddeusJiang/save_it + Give a star ⭐ if you like it, #{Application.get_env(:save_it, :github_url)}
146-161: Consider improving error handling and user feedback.While the basic functionality is correct, consider these improvements:
- Add error handling for the
ExGram.get_me()call- Provide more specific error messages for different failure scenarios
def handle({:command, :delete, %{chat: chat, message_id: message_id, from: from, reply_to_message: reply_to_message}}, _ctx) do - {:ok, %{id: bot_id, username: bot_username}} = ExGram.get_me() + case ExGram.get_me() do + {:ok, %{id: bot_id, username: bot_username}} -> + if Enum.member?([bot_id, from.id], reply_to_message.from.id) do + handle_delete_command(chat.id, message_id, reply_to_message) + else + send_message(chat.id, "Only delete messages from @#{bot_username} and yourself.") + end - if Enum.member?([bot_id, from.id], reply_to_message.from.id) do - handle_delete_command(chat.id, message_id, reply_to_message) - else - send_message(chat.id, "Only delete messages from @#{bot_username} and yourself.") + {:error, _reason} -> + send_message(chat.id, "Unable to verify bot identity. Please try again later.") end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
lib/save_it/bot.ex(4 hunks)lib/save_it/migration/typesense.ex(1 hunks)lib/save_it/migration/typesense/photo.ex(2 hunks)lib/save_it/photo_service.ex(1 hunks)lib/small_sdk/typesense.ex(1 hunks)lib/small_sdk/web_downloader.ex(1 hunks)priv/typesense/2024-10-29_photos_url_to_file_id.ex(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- priv/typesense/2024-10-29_photos_url_to_file_id.ex
🔇 Additional comments (7)
lib/save_it/migration/typesense/photo.ex (2)
70-71: Verify error handling strategy across the module.
The removal of the bang (!) from drop_photos suggests a change in error handling strategy, but other functions like reset! still use the bang convention. Consider maintaining a consistent approach across the module.
Let's check the error handling pattern across the module:
#!/bin/bash
# Description: Check for inconsistent error handling patterns
# Look for mix of bang/non-bang function definitions
ast-grep --pattern 'def $fn!($$$) { $$$ }'
# Look for error handling patterns
rg -A 3 '(rescue|catch|throw|raise|error)'Consider either:
- Keeping the bang and explicitly handling errors:
- def drop_photos() do
- Typesense.delete_collection(@collection_name)
+ def drop_photos!() do
+ case Typesense.delete_collection(@collection_name) do
+ {:ok, _} -> :ok
+ {:error, reason} -> raise "Failed to drop photos: #{inspect(reason)}"
+ end- Or removing bangs consistently across the module and handling errors at a higher level.
40-41: 🛠️ Refactor suggestion
Consider making file_id required since it replaces url.
The schema migration marks file_id as optional, but since it's replacing the url field, it should probably be required to maintain data consistency. This is especially important since the data migration in migrate_photos_data_20241029 assumes all URLs will have a valid file_id in their last segment.
Let's verify the URL format assumption:
Consider this schema update instead:
- %{"name" => "file_id", "type" => "string", "optional" => true},
+ %{"name" => "file_id", "type" => "string"},
%{"name" => "url", "drop" => true}lib/save_it/photo_service.ex (2)
39-45: Verify integration with the /delete command
The new deletion functions look good but need verification of their usage in the bot command implementation.
Let's check how these functions are used in the bot module:
#!/bin/bash
# Check usage of delete functions in the bot module
echo "Checking usage of delete_photo function:"
rg "PhotoService.delete_photo" lib/save_it/bot.ex -A 5
echo "Checking usage of delete_photos function:"
rg "PhotoService.delete_photos" lib/save_it/bot.ex -A 539-41:
Add error handling and input validation
The function should follow the module's error handling pattern and include input validation:
- The function should follow the module's naming convention with
!suffix as it may raise errors - Add validation for
file_idparameter - Handle and return the query result
- def delete_photo(file_id) do
+ def delete_photo!(file_id) when is_binary(file_id) do
+ result = Typesense.delete_document_by_query("photos", "file_id:[#{file_id}]")
+ case result do
+ {:ok, response} -> response
+ {:error, error} -> raise "Failed to delete photo: #{inspect(error)}"
+ end
endLet's verify the error handling pattern in other functions:
lib/small_sdk/typesense.ex (1)
66-71: Verify callers of the renamed function.
The function has been renamed from delete_document! to delete_document, which better reflects its error handling behavior (returns nil for 404 responses). However, this change might affect callers expecting consistent exception behavior.
Consider adding @doc documentation to clarify the error handling behavior:
@doc """
Deletes a document from the specified collection.
Returns:
- The response body on successful deletion
- `nil` if the document is not found
- Raises for other error cases
"""✅ Verification successful
Function rename is safe and doesn't require documentation updates
The search results show that:
- There are no existing calls to the old
delete_document!function - The new
delete_documentfunction is not directly used in the codebase - Only the new
delete_document_by_queryfunction is being used inphoto_service.ex
Since there are no callers of the old delete_document! function, the rename is safe and won't impact any existing code. The suggestion for adding documentation, while good practice, is not critical as the function's behavior is straightforward and follows Elixir conventions where non-bang functions are expected to handle errors gracefully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for callers of the old function name to ensure they're updated
rg "delete_document!" --type elixir
# Search for documentation mentioning the old function name
rg "delete_document!" --type md
Length of output: 71
Script:
#!/bin/bash
# Let's try a broader search to find any references to delete_document
rg "delete_document" --type elixir -B2 -A2
# Also search for test files that might be using this function
fd "test" --type f --extension exs --extension ex --exec rg "delete_document" {} \;
# Look for any documentation in the module itself
rg "@doc.*delete.*document" --type elixir
Length of output: 1329
lib/save_it/bot.ex (2)
31-31: LGTM! Command declarations are clear and well-documented.
The new /delete command and updated /about command descriptions accurately reflect their functionality.
Also applies to: 37-37
143-144: LGTM! Clear error message for the /similar command.
The error message clearly instructs users on how to use the command correctly.
| def delete_collection(collection_name) do | ||
| req = build_request("/collections/#{collection_name}") | ||
| res = Req.delete!(req) | ||
| res = Req.delete(req) | ||
|
|
||
| Typesense.handle_response!(res) | ||
| Typesense.handle_response(res) |
There was a problem hiding this comment.
💡 Codebase verification
Error handling mismatch detected in caller
The drop_photos/0 function in lib/save_it/migration/typesense/photo.ex calls delete_collection/1 but doesn't handle its error cases, while being called by reset!/0 which uses the bang convention (indicating it should raise on errors). This creates an inconsistent error handling pattern.
drop_photos/0should be renamed todrop_photos!/0and handle the response fromdelete_collection/1by raising on error- Alternatively, remove the bang from
reset!/0and propagate the error handling up the chain
Example fix for the first approach:
def drop_photos!() do
case Typesense.delete_collection(@collection_name) do
{:ok, response} -> response
{:error, reason} -> raise "Failed to drop photos collection: #{inspect(reason)}"
end
end🔗 Analysis chain
LGTM! Consistent error handling approach.
The change from bang (!) to non-bang methods is consistent throughout the function, making error handling more graceful.
Let's verify the impact on callers of this function:
Consider adding @doc and @spec annotations to document the possible error cases and return values, for example:
@doc """
Deletes a Typesense collection.
Returns {:ok, response} on success or {:error, reason} on failure.
"""
@spec delete_collection(String.t()) :: {:ok, map()} | {:error, any()}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of delete_collection to ensure they handle the new error cases
rg -A 3 "delete_collection" --type elixir
Length of output: 553
Script:
#!/bin/bash
# Let's check the implementation of reset! in photo.ex to understand how it handles the response
rg -B 2 -A 5 "reset!" lib/save_it/migration/typesense/photo.ex
Length of output: 178
Script:
#!/bin/bash
# Let's also check if there are any other files that might be using similar patterns
ast-grep --pattern 'def reset!() do
$$$
end'
Length of output: 371
Script:
#!/bin/bash
# Let's see the full content of photo.ex to understand the context
cat lib/save_it/migration/typesense/photo.ex
Length of output: 2012
| drop_photos() | ||
| create_photos_20241024!() | ||
| migrate_photos_20241029!() |
There was a problem hiding this comment.
Consider making reset operations atomic.
The reset! function combines multiple operations (drop_photos, create_photos_20241024!, migrate_photos_20241029!) without handling partial failures. If any step fails after drop_photos, the system could be left in an inconsistent state.
Consider implementing a more robust reset operation:
def reset!() do
- drop_photos()
- create_photos_20241024!()
- migrate_photos_20241029!()
+ with :ok <- drop_photos(),
+ :ok <- create_photos_20241024!(),
+ :ok <- migrate_photos_20241029!() do
+ :ok
+ else
+ error ->
+ Logger.error("Reset failed: #{inspect(error)}")
+ error
+ end
endCommittable suggestion skipped: line range outside the PR's diff.
| def delete_photos(file_ids) do | ||
| Typesense.delete_document_by_query("photos", "file_id:[#{Enum.join(file_ids, ",")}]") | ||
| end |
There was a problem hiding this comment.
Improve bulk deletion implementation
The bulk deletion function needs several improvements for robustness and safety:
- Add proper error handling and validation
- Consider chunking for large lists
- Validate each file_id in the list
- def delete_photos(file_ids) do
+ def delete_photos!(file_ids) when is_list(file_ids) do
+ unless Enum.all?(file_ids, &is_binary/1) do
+ raise ArgumentError, "All file_ids must be strings"
+ end
+
+ if length(file_ids) > 100 do
+ Logger.warn("Attempting to delete #{length(file_ids)} photos at once")
+ end
+
+ result = Typesense.delete_document_by_query(
+ "photos",
+ "file_id:[#{Enum.join(file_ids, ",")}]"
+ )
+
+ case result do
+ {:ok, response} -> response
+ {:error, error} -> raise "Failed to delete photos: #{inspect(error)}"
+ end
endConsider implementing batch processing for large lists to avoid potential timeouts or performance issues:
def delete_photos_in_batches!(file_ids, batch_size \\ 100) do
file_ids
|> Enum.chunk_every(batch_size)
|> Enum.map(&delete_photos!/1)
end| # docs: https://typesense.org/docs/27.1/api/documents.html#delete-documents | ||
| def delete_document_by_query(collection_name, filter_by, opts \\ []) do | ||
| batch_size = Keyword.get(opts, :batch_size, 100) | ||
|
|
||
| req = build_request("/collections/#{collection_name}/documents") | ||
|
|
||
| Req.delete(req, params: %{"filter_by" => filter_by, "batch_size" => batch_size}) | ||
| |> handle_response() | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and improve error handling for batch deletions.
While the implementation follows the Typesense API, there are several areas for improvement:
- No validation of the
filter_byparameter - No handling of partial deletion failures
- No progress tracking for large deletions
Consider these improvements:
# docs: https://typesense.org/docs/27.1/api/documents.html#delete-documents
+@doc """
+Deletes documents matching the given filter query.
+
+## Parameters
+ - collection_name: The name of the collection
+ - filter_by: Filter query in Typesense syntax (e.g., "user_id:=1234")
+ - opts: Keyword list of options
+ - batch_size: Number of documents to delete per batch (default: 100)
+
+## Examples
+ delete_document_by_query("photos", "user_id:=1234")
+ delete_document_by_query("photos", "created_at:<1234567890", batch_size: 50)
+"""
def delete_document_by_query(collection_name, filter_by, opts \\ []) do
+ unless is_binary(filter_by) and filter_by != "" do
+ raise ArgumentError, "filter_by must be a non-empty string"
+ end
+
batch_size = Keyword.get(opts, :batch_size, 100)
req = build_request("/collections/#{collection_name}/documents")
-
- Req.delete(req, params: %{"filter_by" => filter_by, "batch_size" => batch_size})
- |> handle_response()
+ case Req.delete(req, params: %{"filter_by" => filter_by, "batch_size" => batch_size}) do
+ {:ok, %{status: status} = response} when status in 200..299 ->
+ response
+ |> handle_response()
+ |> Map.get("num_deleted", 0)
+ error ->
+ Logger.error("Batch deletion failed: #{inspect(error)}")
+ handle_response(error)
+ end
endFor large collections, consider adding progress tracking:
# Add this helper function
defp log_deletion_progress(deleted, total) do
Logger.info("Deleted #{deleted} out of #{total} documents (#{round(deleted/total * 100)}%)")
endFor very large collections, consider implementing this as a background job with progress tracking and the ability to resume on failure.
| defp handle_delete_command(chat_id, message_id, reply_to_message) do | ||
| case reply_to_message do | ||
| %{photo: nil} -> | ||
| delete_message(chat_id, reply_to_message.message_id) | ||
|
|
||
| %{photo: photo} -> | ||
| photo | ||
| |> Enum.map(& &1.file_id) | ||
| |> PhotoService.delete_photos() | ||
|
|
||
| delete_message(chat_id, reply_to_message.message_id) | ||
|
|
||
| _ -> | ||
| send_message(chat_id, "reply a message with /delete command.") | ||
| end | ||
|
|
||
| delete_message(chat_id, message_id) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and message deletion order.
The current implementation has several areas for improvement:
- Photo deletion could fail silently
- Message deletion order might cause confusion if one deletion fails
- No logging for failures
defp handle_delete_command(chat_id, message_id, reply_to_message) do
+ # Delete command message first to provide immediate feedback
+ with {:ok, _} <- delete_message(chat_id, message_id) do
case reply_to_message do
%{photo: nil} ->
- delete_message(chat_id, reply_to_message.message_id)
+ case delete_message(chat_id, reply_to_message.message_id) do
+ {:ok, _} -> :ok
+ {:error, reason} ->
+ Logger.error("Failed to delete message: #{inspect(reason)}")
+ send_message(chat_id, "Failed to delete the message. Please try again.")
+ end
%{photo: photo} ->
- photo
- |> Enum.map(& &1.file_id)
- |> PhotoService.delete_photos()
-
- delete_message(chat_id, reply_to_message.message_id)
+ file_ids = Enum.map(photo, & &1.file_id)
+ case PhotoService.delete_photos(file_ids) do
+ {:ok, _} ->
+ case delete_message(chat_id, reply_to_message.message_id) do
+ {:ok, _} -> :ok
+ {:error, reason} ->
+ Logger.error("Failed to delete message: #{inspect(reason)}")
+ send_message(chat_id, "Photos deleted but failed to delete the message.")
+ end
+ {:error, reason} ->
+ Logger.error("Failed to delete photos: #{inspect(reason)}")
+ send_message(chat_id, "Failed to delete photos. Please try again.")
+ end
_ ->
send_message(chat_id, "reply a message with /delete command.")
end
-
- delete_message(chat_id, message_id)
+ else
+ {:error, reason} ->
+ Logger.error("Failed to delete command message: #{inspect(reason)}")
+ send_message(chat_id, "Failed to process delete command. Please try again.")
+ end
endCommittable suggestion skipped: line range outside the PR's diff.
demo
CleanShot.0006-11-06.at.18.12.00.mp4
Summary by CodeRabbit
New Features
deletecommand for users to delete messages by replying to them.Improvements
aboutcommand and refined responses for other commands.download_filesfunction for better clarity on download outcomes.Bug Fixes