放弃在 save_it bot 添加 notes 功能，保持 save_it 简单 #22
Conversation
WalkthroughThe pull request introduces several changes across multiple files. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (17)
priv/typesense/2024-10-25_create_notes_collections.exs (1)
1-1: Consider adding error handling and idempotency checks.While the migration creates the notes collection, it could benefit from additional safeguards:
- Error handling to gracefully handle failures
- Idempotency checks to prevent duplicate collection creation
- Verification of successful creation
Consider refactoring to include these safety measures:
-Migration.Typesense.Note.create_collection!() +try do + unless Migration.Typesense.Note.collection_exists?() do + Migration.Typesense.Note.create_collection!() + + # Verify creation was successful + unless Migration.Typesense.Note.collection_exists?() do + raise "Notes collection was not created successfully" + end + + IO.puts("Notes collection created successfully") + else + IO.puts("Notes collection already exists") + end +rescue + e -> + IO.puts("Failed to create notes collection: #{inspect(e)}") + raise e +endpriv/typesense/2024-10-24_create_photos_collection.exs (1)
1-1: Consider enhancing migration script robustness.While the basic functionality is correct, consider improving the migration script with error handling and status reporting.
Here's a suggested improvement:
-Migration.Typesense.Photo.create_collection!() +try do + case Migration.Typesense.Photo.create_collection!() do + {:ok, _} -> + IO.puts("Successfully created photos collection") + {:error, %{status_code: 409}} -> + IO.puts("Photos collection already exists, skipping creation") + {:error, error} -> + IO.puts("Failed to create photos collection: #{inspect(error)}") + exit({:shutdown, 1}) + end +rescue + e -> + IO.puts("Unexpected error creating photos collection: #{inspect(e)}") + exit({:shutdown, 1}) +endThis enhancement:
- Provides feedback about migration status
- Handles the case where collection already exists
- Properly surfaces and logs errors
- Ensures process exits with non-zero status on failure
priv/typesense/reset.exs (1)
1-1: Add warning about destructive operation.The script performs destructive operations by resetting collections. This should be clearly documented.
+# WARNING: This script will DELETE and recreate all Typesense collections. +# Make sure you have backups if needed before running this script. + # mix run priv/typesense/reset.exsdocs/dev-logs/2024-10-25.md (2)
3-3: Remove trailing period from headingThe heading contains a trailing period which violates markdown best practices.
-## Req call typesense API alway :timeout, but typesense was updated. +## Req call typesense API alway :timeout, but typesense was updated🧰 Tools
🪛 Markdownlint
3-3: Punctuation: '.'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
5-9: Consider implementing retry mechanism with exponential backoffThe timeout error in the Typesense API calls could be handled more gracefully. Since this is occurring during collection creation, which is a critical operation, consider implementing:
- A retry mechanism with exponential backoff
- Configurable timeout values
- Health checks before migration runs
This would make the system more resilient to temporary network issues or Typesense service delays.
Would you like me to help implement a retry mechanism with proper error handling for the
create_collection!/1function?.gitignore (1)
33-38: Consider adding common environment-specific patterns.While the current environment patterns are good, consider adding common patterns for environment-specific files to ensure comprehensive coverage.
Add these additional patterns:
# scripts _local* _dev* _stag* _prod* +*.env +.env.* +!.env.example nohup.outThis addition will:
- Ignore all
.envfiles- Ignore environment-specific
.envfiles- Keep
.env.examplein version control as a templatelib/migration/typesense.ex (1)
1-42: Consider architectural improvements for better API design.The module provides a good foundation but could benefit from these improvements:
- Add non-bang versions of functions that return
{:ok, result}or{:error, reason}- Consider implementing retry mechanism for transient failures
- Add telemetry for monitoring API calls
Example structure for non-bang function:
def create_collection(schema) do case create_collection!(schema) do result when is_map(result) -> {:ok, result} rescue e in RuntimeError -> {:error, e.message} e in ArgumentError -> {:error, e.message} end endConsider using a library like
retryfor handling transient failures:def create_collection!(schema) do Retry.retry with: exponential_backoff() |> cap(1_000) |> expiry(5_000) do # existing implementation end endlib/migration/typesense/note.ex (1)
4-20: Consider translating comments to English and improving documentation.The schema includes important implementation decisions in Chinese. For better collaboration, consider translating these comments to English.
Here's a suggested translation and documentation improvement:
@notes_schema %{ "name" => "notes", "fields" => [ - # TODO: 第一步先实现文本，今后再考虑图片 + # TODO: First implement text notes, consider adding image support later %{"name" => "content", "type" => "string"}, - # references photos.id - # note: 抉择：这个 app 核心是给予图片的视觉笔记，暂时不考虑单独 text 的笔记 - # %{"name" => "photo_id", "type" => "string"}, - # note: 既然不能实现 RDB reference，那么就直接存储 file_id + # Note: This app focuses on visual notes attached to images + # We store file_id directly since RDB-style references aren't supported %{"name" => "message_id", "type" => "string"},lib/migration/typesense/photo.ex (3)
1-2: Consider using a more specific alias name.The current alias
Migration.Typesensecould be renamed to something more specific likeMigration.Typesense.CoreorMigration.Typesense.Clientto better indicate its role.
4-25: Consider adding indexes for query performance.The schema includes fields that will likely be used for filtering (
belongs_to_id,file_id) but doesn't specify indexing strategies. Consider adding appropriate index configurations to optimize query performance.%{"name" => "file_id", "type" => "string"}, -%{"name" => "belongs_to_id", "type" => "string"}, +%{"name" => "belongs_to_id", "type" => "string", "index" => true},
27-34: Add documentation and error handling for public functions.The public functions would benefit from:
@docand@specannotations to document their purpose and expected behavior- Error handling for collection operations
- Warning about potential data loss in
reset!/0Here's a suggested improvement:
+@doc """ +Creates a new photos collection in Typesense. +Raises an error if the collection already exists. +""" +@spec create_collection!() :: {:ok, map()} | {:error, term()} def create_collection!() do Typesense.create_collection!(@photos_schema) end +@doc """ +WARNING: This function will delete all existing data in the photos collection. +Resets the photos collection by deleting and recreating it. +""" +@spec reset!() :: {:ok, map()} | {:error, term()} def reset!() do - Typesense.delete_collection!(@photos_schema["name"]) - Typesense.create_collection!(@photos_schema) + with {:ok, _} <- Typesense.delete_collection!(@photos_schema["name"]), + {:ok, result} <- Typesense.create_collection!(@photos_schema) do + {:ok, result} + end endlib/save_it/note_service.ex (1)
1-5: Add module and function documentation.Consider adding
@moduledocand@docattributes to improve code documentation and maintainability. This helps other developers understand the purpose and usage of the module and its functions.Example:
@moduledoc """ Provides functionality for managing notes within the SaveIt system. Handles creation, updates, and retrieval of notes using Typesense as the storage backend. """README.md (1)
58-60: Add prerequisites and configuration details for Typesense.Consider adding:
- Minimum Docker version requirements
- Link to Typesense documentation
- Configuration options for production deployment
lib/save_it/photo_service.ex (1)
1-1: Add module documentation.Consider adding
@moduledocand@docattributes to document the module's purpose and the expected response structure from Typesense.+ @moduledoc """ + Service module for managing photos in Typesense. + Handles creation, updates, and search operations for photos. + """ + @typedoc """ + Represents the Typesense response structure: + %{ + "results" => [ + %{ + "hits" => [ + %{"document" => photo_document} + ] + } + ] + } + """ + @type typesense_response :: map()zeabur/template.yaml (1)
61-70: Add health check and resource configurations.The Typesense service configuration could be enhanced with:
- Health check endpoint configuration
- Resource limits to prevent resource exhaustion
Consider adding:
healthCheck: path: /health port: 8108 resources: limits: memory: "1Gi" cpu: "1" requests: memory: "512Mi" cpu: "0.5"lib/small_sdk/typesense.ex (1)
71-75: Add documentation for functions that may raise exceptions.Functions with
!suffix should document their error conditions.Add documentation like this:
+ @doc """ + Updates a document in the specified collection. + + ## Examples + + update_document!("photos", "123", %{title: "New Title"}) + + ## Raises + + * `TypesenseError` - When the document doesn't exist or the update is invalid + """ def update_document!(collection_name, document_id, update_input) dolib/save_it/bot.ex (1)
140-142: Remove commented-out code to improve readabilityThe commented-out code in lines 140-142 is unnecessary and can be removed to keep the codebase clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- .gitignore (1 hunks)
- README.md (2 hunks)
- config/runtime.exs (0 hunks)
- docs/dev-logs/2024-10-25.md (1 hunks)
- docs/dev/typesense.md (1 hunks)
- lib/migration/typesense.ex (1 hunks)
- lib/migration/typesense/note.ex (1 hunks)
- lib/migration/typesense/photo.ex (1 hunks)
- lib/save_it/bot.ex (12 hunks)
- lib/save_it/note_service.ex (1 hunks)
- lib/save_it/photo_service.ex (4 hunks)
- lib/small_sdk/typesense.ex (1 hunks)
- lib/small_sdk/typesense_admin.ex (0 hunks)
- priv/typesense/2024-10-24_create_photos_collection.exs (1 hunks)
- priv/typesense/2024-10-25_create_notes_collections.exs (1 hunks)
- priv/typesense/reset.exs (1 hunks)
- zeabur/template.yaml (1 hunks)
💤 Files with no reviewable changes (2)
- config/runtime.exs
- lib/small_sdk/typesense_admin.ex
🧰 Additional context used
🪛 Markdownlint
docs/dev-logs/2024-10-25.md
3-3: Punctuation: '.'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
docs/dev/typesense.md
6-6: Column: 6
Hard tabs(MD010, no-hard-tabs)
7-7: Column: 6
Hard tabs(MD010, no-hard-tabs)
8-8: Column: 6
Hard tabs(MD010, no-hard-tabs)
9-9: Column: 6
Hard tabs(MD010, no-hard-tabs)
10-10: Column: 6
Hard tabs(MD010, no-hard-tabs)
11-11: Column: 6
Hard tabs(MD010, no-hard-tabs)
14-14: null
Bare URL used(MD034, no-bare-urls)
5-5: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
🔇 Additional comments (13)
priv/typesense/2024-10-25_create_notes_collections.exs (1)
1-1: Verify integration with photo management.Based on the AI summary, this change is part of a larger feature involving both notes and photos. Ensure that the collections can work together as intended.
✅ Verification successful
Let me check the content of the file that shows a relationship between notes and photos to better understand the integration.
Let me check the note collection schema since the previous command had a regex error.
Let me check the NoteService implementation to understand how notes and photos are integrated.
Based on the code analysis, I can now provide a final response:
Integration between notes and photos is properly implemented
The codebase shows a well-defined relationship between notes and photos:
- Notes are attached to photos through the
file_idfield- The bot.ex implementation handles the
/notecommand specifically for photos- Notes can only be added as replies to photos, ensuring proper linking
- The integration is one-directional (notes reference photos) and doesn't require complex collection dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any cross-references between notes and photos collections # to verify their integration echo "Searching for note-photo relationships in the codebase..." rg -l "(?i)(note.*photo|photo.*note)" --type elixir echo "Checking for any shared schema definitions..." ast-grep --pattern 'schema = %{ $$$ photo $$$ note $$$ }'Length of output: 368
Script:
#!/bin/bash # Check the content of the file that showed up in our search echo "Checking content of lib/save_it/bot.ex for note-photo relationships..." cat lib/save_it/bot.ex # Also let's check the note collection schema to see if there are any photo-related fields echo -e "\nChecking note collection schema..." rg -A 10 "schema.*=.*%{" priv/typesense/2024-10-25_create_notes_collections.exsLength of output: 16075
Script:
#!/bin/bash # Check the note collection schema definition echo "Checking note collection schema..." cat priv/typesense/2024-10-25_create_notes_collections.exs # Also let's check the NoteService module to understand the integration echo -e "\nChecking NoteService implementation..." rg -A 5 "defmodule.*NoteService" --type elixirLength of output: 593
Script:
#!/bin/bash # Check the complete NoteService implementation echo "Checking complete NoteService implementation..." cat lib/save_it/note_service.ex # Also let's check the Note migration module to see the schema definition echo -e "\nChecking Note migration module..." fd "note.ex" priv/typesense -x cat {}Length of output: 1588
priv/typesense/2024-10-24_create_photos_collection.exs (1)
1-1: Verify collection schema configuration.The migration creates a photos collection, but we should verify that the schema is properly configured for the intended use case.
Let's check the collection schema definition:
✅ Verification successful
Let me gather more information about the schema fields to ensure all necessary fields are properly configured.
Let me fix the regex pattern and try again.
Let me try a simpler approach to get the schema definition.
Schema configuration is properly set up for photo collection
The schema is well-configured with all necessary fields for the intended use case:
- Image field with CLIP embedding generation configured
- Essential metadata fields: caption, file_id, belongs_to_id
- Proper timestamp field (inserted_at) set as default sorting field
- Appropriate field types and options (optional, facet, store) are defined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the Photo module to verify collection schema configuration rg -l "defmodule.*Photo" | xargs rg "schema|field|index" -A 5Length of output: 1817
Script:
#!/bin/bash # Get the complete schema definition rg -A 20 '@photos_schema %{' lib/migration/typesense/photo.exLength of output: 161
Script:
#!/bin/bash # Get the complete schema definition with proper escaping rg -A 20 "@photos_schema \%\{" lib/migration/typesense/photo.exLength of output: 166
Script:
#!/bin/bash # Get the complete file content to analyze the schema cat lib/migration/typesense/photo.exLength of output: 1031
priv/typesense/reset.exs (1)
3-4: Verify the existence of migration modules.The script references two modules that should exist in the codebase.
✅ Verification successful
Let me check the implementation of these modules to verify they have the
reset!function and understand their behavior.
Both migration modules exist and implement reset! functions correctly
The verification confirms:
- Both
Migration.Typesense.PhotoandMigration.Typesense.Notemodules exist- Each module implements a
reset!function that:
- Deletes the existing collection
- Creates a new collection with the defined schema
- The functions use bang (!) notation consistently, indicating they will raise exceptions on failure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the migration modules # Test: Search for the Photo migration module echo "Searching for Photo migration module..." rg -l "defmodule.*Migration\.Typesense\.Photo" # Test: Search for the Note migration module echo "Searching for Note migration module..." rg -l "defmodule.*Migration\.Typesense\.Note"Length of output: 337
Script:
#!/bin/bash # Check implementation of reset! functions in both modules echo "Checking Photo.reset! implementation:" rg "def reset!" lib/migration/typesense/photo.ex -A 5 echo -e "\nChecking Note.reset! implementation:" rg "def reset!" lib/migration/typesense/note.ex -A 5Length of output: 550
.gitignore (1)
29-32: 🛠️ Refactor suggestionConsider consolidating redundant patterns in the data section.
The specific
_localentry is redundant as it would be matched by the wildcard pattern_local*in the scripts section.Apply this diff to remove the redundancy:
# data -_local dataLikely invalid or redundant comment.
lib/migration/typesense/note.ex (1)
1-2: LGTM! Module structure follows Elixir conventions.lib/migration/typesense/photo.ex (2)
8-18: Verify the CLIP model configuration.The embedding configuration uses the CLIP model 'ts/clip-vit-b-p32'. Please ensure this is the optimal model for your use case, considering:
- Model size and performance requirements
- Image types you'll be processing
- Required embedding dimensions
1-35: Verify access control implementation.Since this collection handles potentially sensitive data:
- Ensure proper access controls are implemented at the application level
- Verify that
belongs_to_idis properly validated before queries- Consider adding a
visibilityfield to support public/private photo accesslib/save_it/note_service.ex (1)
41-62: Review data consistency and search efficiency.The current implementation has several concerns:
- Multiple notes for the same message_id and chat_id suggest potential data consistency issues
- The search query could be vulnerable to injection if the IDs contain special characters
- The search might not be using the most efficient index structure
Let's verify the Typesense collection schema and indexes:
Consider these improvements:
- Add a unique compound index on (message_id, belongs_to_id) to prevent duplicates
- Use proper parameter escaping in the filter_by clause
- Consider implementing a cleanup function to handle duplicate notes
Example schema improvement:
# In your schema definition %{ name: "notes", fields: [ %{name: "message_id", type: "string", facet: true}, %{name: "belongs_to_id", type: "string", facet: true} ], default_sorting_field: "updated_at", symbols_to_index: [], token_separators: [] }README.md (1)
5-9: LGTM! Clear and concise feature list.The new Features section effectively communicates the core capabilities of the bot.
lib/save_it/photo_service.ex (1)
22-22: Consider error handling strategy with bang function.The change to
update_document!means errors will be raised instead of returned. Ensure calling code can handle these exceptions appropriately.Let's check for error handling in the calling code:
✅ Verification successful
Let me gather more context about the error handling patterns in the codebase:
Let me try one more search to find error handling patterns, fixing the regex:
Based on the bot implementation, I can now generate the final response:
Bang functions are appropriately used in this context
The use of
update_document!inPhotoServiceis consistent with the application's error handling strategy. The bot handles potential errors at the boundary (controller) level, where it:
- Uses pattern matching and case statements for explicit error handling
- Logs errors appropriately (e.g.,
Logger.error)- Provides user-friendly error messages through
send_messageExamples from the bot implementation:
- Error handling in photo search:
case photos do [] -> send_message(...) end- Error handling in media group sending:
case ExGram.send_media_group(...) do {:error, reason} -> Logger.error(...) end🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to update_photo to verify error handling rg -A 5 "update_photo" --type elixirLength of output: 380
Script:
#!/bin/bash # Search for try/rescue blocks and error handling patterns rg -B 2 -A 5 "try|rescue|catch|\!" --type elixir # Look for supervisor or application startup code fd "application.ex|supervisor.ex" --type f --exec cat {}Length of output: 795
Script:
#!/bin/bash # Search for try/rescue blocks and error handling patterns with fixed regex rg -B 2 -A 5 "(try|rescue|catch|\!)" --type elixir # Look for the bot implementation that might be calling photo_service fd "bot.ex" --type f --exec cat {}Length of output: 15838
zeabur/template.yaml (3)
41-55: Enhance environment variable configuration.The current configuration for sensitive environment variables could be improved:
- Empty default values could lead to silent failures
- No validation or format requirements are specified
Consider:
- Adding validation rules or format requirements for tokens
- Making required environment variables explicit
- Adding descriptions for each environment variable
Example enhancement:
env: TELEGRAM_BOT_TOKEN: required: true description: "Telegram Bot API token from BotFather" pattern: "^[0-9]{8,10}:[a-zA-Z0-9_-]{35}$" expose: false
76-78:⚠️ Potential issueReview environment variable exposure.
The TYPESENSE_API_KEY is marked as
expose: truewhich might expose sensitive information unnecessarily.Consider setting
expose: falseunless there's a specific requirement to expose this value:TYPESENSE_API_KEY: default: ${PASSWORD} - expose: true + expose: false
8-8: Consider using permanent, organization-owned asset URLs.The current configuration uses potentially temporary user-attachment URLs for assets:
- Icon URL on line 8
- Empty coverImage field on line 14-15
- Video URL on line 28
These URLs might become invalid if the user attachments are cleaned up or the user account is removed.
Consider:
- Moving these assets to your organization's repository or a CDN
- Either populate or remove the empty coverImage field
- Using a more permanent video hosting solution
Also applies to: 14-15, 28-28
| Migration.Typesense.Photo.reset!() | ||
| Migration.Typesense.Note.reset!() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and logging.
Since these are destructive operations that reset collections, it would be beneficial to:
- Add error handling to gracefully handle failures
- Add logging to track the reset progress
# mix run priv/typesense/reset.exs
-Migration.Typesense.Photo.reset!()
-Migration.Typesense.Note.reset!()
+IO.puts("Starting Typesense collections reset...")
+
+try do
+ IO.puts("Resetting photos collection...")
+ Migration.Typesense.Photo.reset!()
+
+ IO.puts("Resetting notes collection...")
+ Migration.Typesense.Note.reset!()
+
+ IO.puts("Successfully reset all collections")
+rescue
+ e in RuntimeError ->
+ IO.puts("Error resetting collections: #{Exception.message(e)}")
+ System.halt(1)
+end📝 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.
| Migration.Typesense.Photo.reset!() | |
| Migration.Typesense.Note.reset!() | |
| IO.puts("Starting Typesense collections reset...") | |
| try do | |
| IO.puts("Resetting photos collection...") | |
| Migration.Typesense.Photo.reset!() | |
| IO.puts("Resetting notes collection...") | |
| Migration.Typesense.Note.reset!() | |
| IO.puts("Successfully reset all collections") | |
| rescue | |
| e in RuntimeError -> | |
| IO.puts("Error resetting collections: #{Exception.message(e)}") | |
| System.halt(1) | |
| end |
lib/migration/typesense.ex
Outdated
| def create_collection!(schema) do | ||
| req = build_request("/collections") | ||
| {:ok, res} = Req.post(req, json: schema) | ||
|
|
||
| res.body | ||
| end |
There was a problem hiding this comment.
Add error handling and documentation for create_collection!.
The function needs better error handling and documentation:
- The
!suffix indicates it raises errors, but the error cases aren't properly handled - The schema parameter should be validated
Consider this improvement:
+ @doc """
+ Creates a new collection in Typesense with the given schema.
+
+ ## Parameters
+ - schema: Map containing the collection schema definition
+
+ ## Returns
+ - Map containing the created collection details
+
+ ## Raises
+ - RuntimeError if the collection creation fails
+ """
def create_collection!(schema) do
+ unless is_map(schema), do: raise ArgumentError, "schema must be a map"
+
req = build_request("/collections")
- {:ok, res} = Req.post(req, json: schema)
+ case Req.post(req, json: schema) do
+ {:ok, res} when res.status in 200..299 -> res.body
+ {:ok, res} -> raise "Failed to create collection: #{inspect(res.body)}"
+ {:error, error} -> raise "Request failed: #{inspect(error)}"
+ end
-
- res.body
end📝 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.
| def create_collection!(schema) do | |
| req = build_request("/collections") | |
| {:ok, res} = Req.post(req, json: schema) | |
| res.body | |
| end | |
| @doc """ | |
| Creates a new collection in Typesense with the given schema. | |
| ## Parameters | |
| - schema: Map containing the collection schema definition | |
| ## Returns | |
| - Map containing the created collection details | |
| ## Raises | |
| - RuntimeError if the collection creation fails | |
| """ | |
| def create_collection!(schema) do | |
| unless is_map(schema), do: raise ArgumentError, "schema must be a map" | |
| req = build_request("/collections") | |
| case Req.post(req, json: schema) do | |
| {:ok, res} when res.status in 200..299 -> res.body | |
| {:ok, res} -> raise "Failed to create collection: #{inspect(res.body)}" | |
| {:error, error} -> raise "Request failed: #{inspect(error)}" | |
| end | |
| end |
lib/migration/typesense.ex
Outdated
| defp get_env() do | ||
| url = Application.fetch_env!(:save_it, :typesense_url) | ||
| api_key = Application.fetch_env!(:save_it, :typesense_api_key) | ||
|
|
||
| {url, api_key} | ||
| end | ||
|
|
||
| defp build_request(path) do | ||
| {url, api_key} = get_env() | ||
|
|
||
| Req.new( | ||
| base_url: url, | ||
| url: path, | ||
| headers: [ | ||
| {"Content-Type", "application/json"}, | ||
| {"X-TYPESENSE-API-KEY", api_key} | ||
| ] | ||
| ) | ||
| end |
There was a problem hiding this comment.
Enhance helper functions for better error handling and security.
The helper functions need improvements:
get_env/0should provide better error messages for missing configurationbuild_request/1should encode the path parameter
Consider these improvements:
defp get_env() do
- url = Application.fetch_env!(:save_it, :typesense_url)
- api_key = Application.fetch_env!(:save_it, :typesense_api_key)
+ url = case Application.fetch_env(:save_it, :typesense_url) do
+ {:ok, value} when is_binary(value) and value != "" -> value
+ _ -> raise "Missing or invalid :typesense_url configuration"
+ end
+
+ api_key = case Application.fetch_env(:save_it, :typesense_api_key) do
+ {:ok, value} when is_binary(value) and value != "" -> value
+ _ -> raise "Missing or invalid :typesense_api_key configuration"
+ end
{url, api_key}
end
defp build_request(path) do
{url, api_key} = get_env()
+ encoded_path = URI.encode(path)
Req.new(
base_url: url,
- url: path,
+ url: encoded_path,
headers: [
{"Content-Type", "application/json"},
{"X-TYPESENSE-API-KEY", api_key}
]
)
end📝 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.
| defp get_env() do | |
| url = Application.fetch_env!(:save_it, :typesense_url) | |
| api_key = Application.fetch_env!(:save_it, :typesense_api_key) | |
| {url, api_key} | |
| end | |
| defp build_request(path) do | |
| {url, api_key} = get_env() | |
| Req.new( | |
| base_url: url, | |
| url: path, | |
| headers: [ | |
| {"Content-Type", "application/json"}, | |
| {"X-TYPESENSE-API-KEY", api_key} | |
| ] | |
| ) | |
| end | |
| defp get_env() do | |
| url = case Application.fetch_env(:save_it, :typesense_url) do | |
| {:ok, value} when is_binary(value) and value != "" -> value | |
| _ -> raise "Missing or invalid :typesense_url configuration" | |
| end | |
| api_key = case Application.fetch_env(:save_it, :typesense_api_key) do | |
| {:ok, value} when is_binary(value) and value != "" -> value | |
| _ -> raise "Missing or invalid :typesense_api_key configuration" | |
| end | |
| {url, api_key} | |
| end | |
| defp build_request(path) do | |
| {url, api_key} = get_env() | |
| encoded_path = URI.encode(path) | |
| Req.new( | |
| base_url: url, | |
| url: encoded_path, | |
| headers: [ | |
| {"Content-Type", "application/json"}, | |
| {"X-TYPESENSE-API-KEY", api_key} | |
| ] | |
| ) | |
| end |
lib/migration/typesense.ex
Outdated
| def delete_collection!(collection_name) do | ||
| req = build_request("/collections/#{collection_name}") | ||
| {:ok, res} = Req.delete(req) | ||
|
|
||
| res.body | ||
| end | ||
|
|
||
| def list_collections() do | ||
| req = build_request("/collections") | ||
| {:ok, res} = Req.get(req) | ||
|
|
||
| res.body | ||
| end |
There was a problem hiding this comment.
Improve error handling and documentation for delete_collection! and list_collections.
Both functions need similar improvements as create_collection!:
- Proper error handling for failed requests
- Input validation for delete_collection!
- Function documentation
Apply these improvements:
+ @doc """
+ Deletes a collection from Typesense by name.
+
+ ## Parameters
+ - collection_name: String name of the collection to delete
+
+ ## Raises
+ - ArgumentError if collection_name is invalid
+ - RuntimeError if the deletion fails
+ """
def delete_collection!(collection_name) do
+ unless is_binary(collection_name) and collection_name != "",
+ do: raise ArgumentError, "collection_name must be a non-empty string"
+
req = build_request("/collections/#{collection_name}")
- {:ok, res} = Req.delete(req)
-
- res.body
+ case Req.delete(req) do
+ {:ok, res} when res.status in 200..299 -> res.body
+ {:ok, res} -> raise "Failed to delete collection: #{inspect(res.body)}"
+ {:error, error} -> raise "Request failed: #{inspect(error)}"
+ end
end
+ @doc """
+ Lists all collections in Typesense.
+
+ ## Returns
+ - List of collection details
+
+ ## Raises
+ - RuntimeError if the request fails
+ """
def list_collections() do
req = build_request("/collections")
- {:ok, res} = Req.get(req)
-
- res.body
+ case Req.get(req) do
+ {:ok, res} when res.status in 200..299 -> res.body
+ {:ok, res} -> raise "Failed to list collections: #{inspect(res.body)}"
+ {:error, error} -> raise "Request failed: #{inspect(error)}"
+ end
end📝 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.
| def delete_collection!(collection_name) do | |
| req = build_request("/collections/#{collection_name}") | |
| {:ok, res} = Req.delete(req) | |
| res.body | |
| end | |
| def list_collections() do | |
| req = build_request("/collections") | |
| {:ok, res} = Req.get(req) | |
| res.body | |
| end | |
| @doc """ | |
| Deletes a collection from Typesense by name. | |
| ## Parameters | |
| - collection_name: String name of the collection to delete | |
| ## Raises | |
| - ArgumentError if collection_name is invalid | |
| - RuntimeError if the deletion fails | |
| """ | |
| def delete_collection!(collection_name) do | |
| unless is_binary(collection_name) and collection_name != "", | |
| do: raise ArgumentError, "collection_name must be a non-empty string" | |
| req = build_request("/collections/#{collection_name}") | |
| case Req.delete(req) do | |
| {:ok, res} when res.status in 200..299 -> res.body | |
| {:ok, res} -> raise "Failed to delete collection: #{inspect(res.body)}" | |
| {:error, error} -> raise "Request failed: #{inspect(error)}" | |
| end | |
| end | |
| @doc """ | |
| Lists all collections in Typesense. | |
| ## Returns | |
| - List of collection details | |
| ## Raises | |
| - RuntimeError if the request fails | |
| """ | |
| def list_collections() do | |
| req = build_request("/collections") | |
| case Req.get(req) do | |
| {:ok, res} when res.status in 200..299 -> res.body | |
| {:ok, res} -> raise "Failed to list collections: #{inspect(res.body)}" | |
| {:error, error} -> raise "Request failed: #{inspect(error)}" | |
| end | |
| end |
lib/migration/typesense/note.ex
Outdated
| def create_collection!() do | ||
| Typesense.create_collection!(@notes_schema) | ||
| end | ||
|
|
||
| def reset!() do | ||
| Typesense.delete_collection!(@notes_schema["name"]) | ||
| Typesense.create_collection!(@notes_schema) | ||
| end |
There was a problem hiding this comment.
Add error handling and documentation for public functions.
The current implementation could benefit from:
- Function documentation
- Error handling for collection operations
- Transaction-like behavior for reset operation
Here's a suggested implementation:
+ @doc """
+ Creates a new notes collection in Typesense.
+ Raises an error if the collection already exists.
+ """
def create_collection!() do
Typesense.create_collection!(@notes_schema)
end
+ @doc """
+ Resets the notes collection by deleting and recreating it.
+ Raises an error if either operation fails.
+ """
def reset!() do
+ with :ok <- Typesense.delete_collection!(@notes_schema["name"]),
+ :ok <- Typesense.create_collection!(@notes_schema) do
+ :ok
+ else
+ error ->
+ # Log the error and re-raise
+ require Logger
+ Logger.error("Failed to reset notes collection: #{inspect(error)}")
+ raise "Failed to reset notes collection"
+ end
- Typesense.delete_collection!(@notes_schema["name"])
- Typesense.create_collection!(@notes_schema)
endCommittable suggestion was skipped due to low confidence.
lib/small_sdk/typesense.ex
Outdated
| def handle_response(res) do | ||
| case res do | ||
| %Req.Response{status: 200} -> | ||
| res.body | ||
|
|
||
| %Req.Response{status: 201} -> | ||
| res.body | ||
|
|
||
| %Req.Response{status: 400} -> | ||
| Logger.error("Bad Request: #{inspect(res.body)}") | ||
| raise "Bad Request" | ||
|
|
||
| %Req.Response{status: 401} -> | ||
| raise "Unauthorized" | ||
|
|
||
| %Req.Response{status: 404} -> | ||
| nil | ||
|
|
||
| %Req.Response{status: 409} -> | ||
| raise "Conflict" | ||
|
|
||
| %Req.Response{status: 422} -> | ||
| raise "Unprocessable Entity" | ||
|
|
||
| %Req.Response{status: 503} -> | ||
| raise "Service Unavailable" | ||
|
|
||
| _ -> | ||
| raise "Unknown error" | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling and adding documentation.
While centralizing response handling is good, the implementation could be improved:
- Consider using structured errors instead of raw strings
- Add module documentation explaining the error handling strategy
- Include response body in error messages for better debugging
Here's a suggested implementation:
+ @moduledoc """
+ Client SDK for Typesense search engine.
+
+ ## Error Handling
+
+ This module raises specific exceptions for different HTTP status codes:
+ - 400: TypesenseBadRequestError
+ - 401: TypesenseUnauthorizedError
+ - 409: TypesenseConflictError
+ - 422: TypesenseUnprocessableEntityError
+ - 503: TypesenseServiceUnavailableError
+ """
+ defmodule TypesenseError do
+ defexception [:message, :status, :body]
+ end
def handle_response(res) do
case res do
%Req.Response{status: status} when status in [200, 201] ->
res.body
%Req.Response{status: 400} = res ->
Logger.error("Bad Request: #{inspect(res.body)}")
- raise "Bad Request"
+ raise TypesenseError,
+ message: "Bad Request",
+ status: 400,
+ body: res.body
%Req.Response{status: 401} = res ->
- raise "Unauthorized"
+ raise TypesenseError,
+ message: "Unauthorized",
+ status: 401,
+ body: res.body
# ... similar changes for other status codesCommittable suggestion was skipped due to low confidence.
lib/small_sdk/typesense.ex
Outdated
| @@ -1,25 +1,78 @@ | |||
| defmodule SmallSdk.Typesense do | |||
There was a problem hiding this comment.
💡 Codebase verification
Tests needed for Typesense functionality
The codebase lacks test coverage for the Typesense module. Please add tests covering:
- HTTP response handling in
handle_response/1 - Search functionality with various parameter combinations
- Error cases in document operations
Suggested test structure:
- Create a new file
test/small_sdk/typesense_test.exs - Consider using mocks or fixtures for HTTP responses
- Include both success and error scenarios
🔗 Analysis chain
Verify test coverage for the new functionality.
The changes introduce new error handling and search functionality. Please ensure there are comprehensive tests covering:
- Different HTTP response scenarios in
handle_response/1 - Search functionality with various parameter combinations
- Error cases in document operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for test files related to this module
fd "typesense_test.exs$" test/
Length of output: 32
Script:
#!/bin/bash
# Let's try a broader search for test files
fd "test.exs$" test/
# Also search for any references to Typesense in test files
rg -l "Typesense" test/
# Check if there are any test helpers or support files
fd "test_helper.exs$" test/
# Look for any mock or fixture data related to Typesense
rg -l "typesense" test/
Length of output: 143
Script:
#!/bin/bash
# Let's examine the content of test_helper.exs for any test setup related to Typesense
cat test/test_helper.exs
# Check the content of save_it_test.exs for any Typesense-related tests
cat test/save_it_test.exs
# Look for any test configuration in config files
fd "config.exs$" config/
Length of output: 249
lib/save_it/bot.ex
Outdated
| case note do | ||
| nil -> send_message(chat.id, "Failed to add note.") | ||
| # TODO:nice_to_have: 添加一个 emoji 即可 | ||
| _ -> send_message(chat.id, "Note added successfully.") | ||
| end |
There was a problem hiding this comment.
Improve error handling when creating a note
Since NoteService.create_note!/1 raises an exception upon failure, the nil case might not occur, and unhandled exceptions could crash the bot. Consider using the non-bang version create_note/1 and handle the {:ok, note} and {:error, reason} tuples to manage errors gracefully.
Apply this diff to handle errors appropriately:
- note =
- NoteService.create_note!(%{
- content: note_content,
- message_id: message_id,
- file_id: file_id,
- belongs_to_id: chat.id
- })
-
- case note do
- nil -> send_message(chat.id, "Failed to add note.")
- # TODO:nice_to_have: 添加一个 emoji 即可
- _ -> send_message(chat.id, "Note added successfully.")
- end
+ case NoteService.create_note(%{
+ content: note_content,
+ message_id: message_id,
+ file_id: file_id,
+ belongs_to_id: chat.id
+ }) do
+ {:ok, _note} ->
+ send_message(chat.id, "Note added successfully.")
+ {:error, reason} ->
+ Logger.error("Failed to add note: #{inspect(reason)}")
+ send_message(chat.id, "Failed to add note.")
+ end📝 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.
| case note do | |
| nil -> send_message(chat.id, "Failed to add note.") | |
| # TODO:nice_to_have: 添加一个 emoji 即可 | |
| _ -> send_message(chat.id, "Note added successfully.") | |
| end | |
| case NoteService.create_note(%{ | |
| content: note_content, | |
| message_id: message_id, | |
| file_id: file_id, | |
| belongs_to_id: chat.id | |
| }) do | |
| {:ok, _note} -> | |
| send_message(chat.id, "Note added successfully.") | |
| {:error, reason} -> | |
| Logger.error("Failed to add note: #{inspect(reason)}") | |
| send_message(chat.id, "Failed to add note.") | |
| end |
lib/save_it/bot.ex
Outdated
| def handle({:edited_message, msg}, _context) do | ||
| %{message_id: message_id, chat: chat, text: text} = msg | ||
|
|
||
| edited_note_text = | ||
| case Regex.run(~r/\/note\s+(.*)/, text) do | ||
| [_, edited_note_text] -> edited_note_text | ||
| _ -> nil | ||
| end | ||
|
|
||
| case String.contains?(text, "/note") do | ||
| true -> | ||
| note = NoteService.get_note!(message_id, chat.id) | ||
|
|
||
| NoteService.update_note!(note["id"], %{"content" => edited_note_text}) | ||
|
|
||
| false -> | ||
| Logger.debug("edited message: #{inspect(_msg)}") | ||
| end |
There was a problem hiding this comment.
Correct variable name in Logger debug statement and handle missing text
In line 382, the variable _msg is undefined; it should be msg as per the function parameter. Additionally, if text is nil, accessing it may cause an error. Safely extract text and handle cases where it might be nil.
Apply this diff to fix the variable name and handle text:
-def handle({:edited_message, msg}, _context) do
- %{message_id: message_id, chat: chat, text: text} = msg
+def handle({:edited_message, msg}, _context) do
+ %{message_id: message_id, chat: chat} = msg
+ text = Map.get(msg, :text)
edited_note_text =
case Regex.run(~r/\/note\s+(.*)/, text) do
[_, edited_note_text] -> edited_note_text
_ -> nil
end
case String.contains?(text, "/note") do
true ->
note = NoteService.get_note!(message_id, chat.id)
NoteService.update_note!(note["id"], %{"content" => edited_note_text})
false ->
- Logger.debug("edited message: #{inspect(_msg)}")
+ Logger.debug("edited message: #{inspect(msg)}")
end
endAlso, add a check to handle when text is nil:
+ if is_nil(text) do
+ Logger.debug("Edited message has no text")
+ return
+ endCommittable suggestion was skipped due to low confidence.
lib/save_it/bot.ex
Outdated
| def handle( | ||
| {:command, :note, | ||
| %{message_id: message_id, chat: chat, text: text, reply_to_message: reply_to_message}}, | ||
| _context | ||
| ) | ||
| when is_binary(text) do | ||
| file_id = reply_to_message.photo |> List.last() |> Map.get(:file_id) | ||
|
|
||
| case file_id do | ||
| nil -> | ||
| send_message(chat.id, "Please reply to a photo to add a note.") | ||
|
|
||
| _ -> | ||
| case String.trim(text) do | ||
| "" -> | ||
| send_message(chat.id, "What note do you want to add?") | ||
|
|
||
| note_content -> | ||
| note = | ||
| NoteService.create_note!(%{ | ||
| content: note_content, | ||
| message_id: message_id, | ||
| file_id: file_id, | ||
| belongs_to_id: chat.id | ||
| }) | ||
|
|
||
| case note do | ||
| nil -> send_message(chat.id, "Failed to add note.") | ||
| # TODO:nice_to_have: 添加一个 emoji 即可 | ||
| _ -> send_message(chat.id, "Note added successfully.") | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Handle cases where reply_to_message might be missing to prevent crashes
The function assumes reply_to_message is always present. If a user sends the /note command without replying to a message, this pattern match will fail, potentially causing a crash. Consider adjusting the pattern matching and adding checks to handle cases where reply_to_message is nil.
Apply this diff to adjust the pattern matching and handle missing reply_to_message:
-def handle(
- {:command, :note,
- %{message_id: message_id, chat: chat, text: text, reply_to_message: reply_to_message}},
- _context
- )
+def handle(
+ {:command, :note,
+ %{message_id: message_id, chat: chat, text: text} = msg},
+ _context
+ )
when is_binary(text) do
+ reply_to_message = Map.get(msg, :reply_to_message)
+ case reply_to_message do
+ nil ->
+ send_message(chat.id, "Please reply to a photo to add a note.")
+ %{photo: photos} ->
+ file_id = photos |> List.last() |> Map.get(:file_id)
+
+ case file_id do
+ nil ->
+ send_message(chat.id, "Please reply to a photo to add a note.")
+ _ ->
+ # Proceed with existing logic
+ end
+ _ ->
+ send_message(chat.id, "Please reply to a photo message.")
+ endCommittable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (23)
priv/typesense/2024-10-24_create_photos_collection.exs (1)
1-1: Consider enhancing migration robustness.While the basic functionality is correct, consider improving the migration script with:
- Error handling and logging
- Idempotency check to prevent duplicate collection creation
- Rollback mechanism for failed migrations
Here's a suggested improvement:
-Migration.Typesense.Photo.create_collection!() +require Logger + +try do + case Migration.Typesense.Photo.create_collection!() do + {:ok, collection} -> + Logger.info("Successfully created photos collection: #{inspect(collection)}") + {:error, %{"message" => "Collection with name photos already exists"}} -> + Logger.info("Photos collection already exists, skipping creation") + {:error, reason} -> + Logger.error("Failed to create photos collection: #{inspect(reason)}") + raise "Migration failed" + end +rescue + e -> + Logger.error("Migration failed: #{inspect(e)}") + raise e +endpriv/typesense/reset.exs (1)
1-1: Enhance script documentation.The current comment only shows how to run the script. Consider adding more context about what the script does and its potential impact.
-# mix run priv/typesense/reset.exs +# Reset all Typesense collections (photos and notes) +# +# WARNING: This script will delete all existing data in these collections! +# +# Usage: +# mix run priv/typesense/reset.exsdocs/dev-logs/2024-10-25.md (2)
3-3: Remove trailing period from headingThe heading should not end with punctuation according to markdown style guidelines.
-## Req call typesense API alway :timeout, but typesense was updated. +## Req call typesense API alway :timeout, but typesense was updated🧰 Tools
🪛 Markdownlint
3-3: Punctuation: '.'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
3-9: Enhance error documentation with more contextThe error log would be more helpful with additional details such as:
- Steps to reproduce the issue
- Environment details (e.g., Typesense version, network configuration)
- Resolution steps or workarounds
- Related configuration settings (e.g., timeout values)
This context would help other developers encountering similar issues.
Would you like me to provide a template for documenting these additional details?
🧰 Tools
🪛 Markdownlint
3-3: Punctuation: '.'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
lib/migration/typesense.ex (1)
1-42: Consider adding resilience patterns for external service communication.The module makes direct HTTP calls to Typesense without implementing common resilience patterns.
Consider implementing:
- Circuit breaker pattern to prevent cascading failures
- Rate limiting to avoid overwhelming the Typesense service
- Logging sanitization to prevent API key exposure
- Metrics collection for monitoring API calls
You could use libraries like:
circuit_breakerfor failure protectionhammerfor rate limitingtelemetryfor metricsWould you like me to provide example implementations for any of these patterns?
lib/migration/typesense/note.ex (4)
7-8: Translate TODO comment to English for better maintainabilityThe TODO comment in Chinese should be translated to English to ensure all team members can understand it.
- # TODO: 第一步先实现文本，今后再考虑图片 + # TODO: First implement text notes, consider adding image support later
10-11: Translate implementation note to EnglishThe implementation note should be in English for consistency and maintainability.
- # note: 抉择：这个 app 核心是给予图片的视觉笔记，暂时不考虑单独 text 的笔记 + # Note: This app primarily focuses on visual notes for images, standalone text notes are not considered for now
12-13: Translate database design note to EnglishThe database design note should be in English.
- # note: 既然不能实现 RDB reference，那么就直接存储 file_id + # Note: Since RDB references cannot be implemented, we'll store the file_id directly
22-29: Document error scenarios for bang functionsBoth functions use the bang (!) convention indicating they raise errors, but the error scenarios aren't documented. Consider adding
@docand@specattributes to clarify:
- Expected error types
- When errors might occur
- How to handle them
Example documentation:
@doc """ Creates a new notes collection in Typesense. Raises `Typesense.Error` if: - Collection already exists - Connection to Typesense fails - Schema validation fails """ @spec create_collection!() :: :ok | no_return() def create_collection!() dolib/migration/typesense/photo.ex (1)
1-3: Consider being more explicit with the alias.Instead of aliasing the entire
Migration.Typesensemodule, consider aliasing only the specific functions you need. This makes the code's dependencies more explicit.- alias Migration.Typesense + import Migration.Typesense, only: [create_collection!: 1, delete_collection!: 1]lib/save_it/note_service.ex (1)
1-1: Add module documentationConsider adding
@moduledocand@docdocumentation to describe the module's purpose, function behaviors, and error conditions. This will help other developers understand how to use this service correctly.Example:
@moduledoc """ Provides functionality for managing notes in the Typesense database. This service handles the creation, updating, and retrieval of notes, with each note being associated with a message and chat. """ @doc """ Creates a new note with the given parameters. ## Parameters - note_params: Map containing :message_id and :belongs_to_id ## Returns - The created note document ## Raises - ArgumentError if message_id or belongs_to_id are not integers - RuntimeError if Typesense operation fails """ def create_note!(%{message_id: message_id, belongs_to_id: belongs_to_id} = note_params)README.md (3)
58-69: Enhance environment variable documentation.Consider adding descriptions for each environment variable to help new contributors:
# modify env -export TELEGRAM_BOT_TOKEN= -export TYPESENSE_URL= -export TYPESENSE_API_KEY= +# Your Telegram Bot Token from BotFather +export TELEGRAM_BOT_TOKEN=your_token_here + +# Typesense server URL (default: http://localhost:8108) +export TYPESENSE_URL=your_typesense_url + +# Typesense API key (default: xyz if using docker-compose) +export TYPESENSE_API_KEY=your_api_key
75-87: Improve security and placeholder values in the shell script example.
- Add a comment warning about keeping the script secure due to sensitive data
- Use clearer placeholder values
#!/bin/sh + +# IMPORTANT: Keep this file secure and never commit it to version control +# Make sure to restrict file permissions: chmod 600 start.sh -export TELEGRAM_BOT_TOKEN=<YOUR_TELEGRAM_BOT_TOKEN> +export TELEGRAM_BOT_TOKEN="123456789:ABCdefGHIjklMNOpqrsTUVwxyz" # Replace with your bot token -export TYPESENSE_URL=<YOUR_TYPESENSE_URL> -export TYPESENSE_API_KEY=<YOUR_TYPESENSE_API_KEY> +export TYPESENSE_URL="http://localhost:8108" # Replace if using remote instance +export TYPESENSE_API_KEY="xyz" # Replace with your API key -export GOOGLE_OAUTH_CLIENT_ID=<YOUR_GOOGLE_OAUTH_CLIENT_ID> -export GOOGLE_OAUTH_CLIENT_SECRET=<YOUR_GOOGLE_OAUTH_CLIENT_SECRET> +export GOOGLE_OAUTH_CLIENT_ID="your-client-id.apps.googleusercontent.com" +export GOOGLE_OAUTH_CLIENT_SECRET="your-client-secret"
71-72: Fix grammar and improve formatting.Consider these improvements:
-Pro Tips: create shell script for fast run app +Pro Tip: Create a shell script for quickly running the app🧰 Tools
🪛 LanguageTool
[uncategorized] ~71-~71: You might be missing the article “a” here.
Context: ...Pro Tips: create shell script for fast run app 1. touch start.shsh #!/bi...(AI_EN_LECTOR_MISSING_DETERMINER_A)
lib/save_it/photo_service.ex (1)
Line range hint
28-74: Consider extracting search configuration for better maintainability.The search functions share similar configuration patterns and response handling. Consider:
- Extracting common search parameters into module attributes or a config struct
- Creating a shared private function for response handling
- Implementing a common search interface that accepts different query builders
Example refactor structure:
defmodule SaveIt.PhotoService.SearchConfig do defstruct collection: "photos", exclude_fields: ["image_embedding"], distance_threshold: 0.4 end # Usage in PhotoService @search_config %SearchConfig{} defp handle_search_response({:ok, res}) do with %{"results" => [%{"hits" => hits} | _]} <- res.body do Enum.map(hits, & &1["document"]) else _ -> [] end endzeabur/template.yaml (3)
14-15: Remove empty coverImage field if not used.The
coverImagefield is defined but empty. Consider either adding a cover image URL or removing the field entirely to maintain clean configuration.
41-55: Enhance environment variable configuration.Several critical environment variables have empty default values:
- TELEGRAM_BOT_TOKEN
- GOOGLE_OAUTH_CLIENT_ID
- GOOGLE_OAUTH_CLIENT_SECRET
Consider:
- Adding validation to ensure these are set before deployment
- Including documentation about required values
- Adding placeholders like
<YOUR_BOT_TOKEN>instead of empty strings
62-66: Review Typesense version and port exposure.A few considerations for the Typesense service configuration:
- The version is pinned to 27.1, which is good for stability but consider documenting the version update strategy
- Port 8108 is exposed as HTTP. Consider:
- Adding rate limiting
- Implementing proper authentication
- Using HTTPS if exposed externally
lib/small_sdk/typesense.ex (2)
40-41: Add documentation for create_document!/2.The function should have documentation explaining its behavior, especially since it can raise exceptions.
+ @doc """ + Creates a document in the specified Typesense collection. + + ## Parameters + - collection_name: The name of the collection + - document: The document to create + + ## Returns + The created document + + ## Raises + Various exceptions based on the API response + """ + @spec create_document!(String.t(), map()) :: map() def create_document!(collection_name, document) do
71-75: Add documentation and input validation for update_document!/3.The function should validate inputs and include documentation.
+ @doc """ + Updates a document in the specified Typesense collection. + + ## Parameters + - collection_name: The name of the collection + - document_id: The ID of the document to update + - update_input: Map containing the fields to update + + ## Returns + The updated document + + ## Raises + Various exceptions based on the API response + """ + @spec update_document!(String.t(), String.t(), map()) :: map() def update_document!(collection_name, document_id, update_input) do + unless is_binary(collection_name) and collection_name != "", + do: raise ArgumentError, "collection_name must be a non-empty string" + unless is_binary(document_id) and document_id != "", + do: raise ArgumentError, "document_id must be a non-empty string" + unless is_map(update_input), + do: raise ArgumentError, "update_input must be a map" + req = build_request("/collections/#{collection_name}/documents/#{document_id}") {:ok, res} = Req.patch(req, json: update_input) handle_response(res) endlib/save_it/bot.ex (3)
140-143: Remove commented-out code to keep the codebase cleanThe code includes commented-out function definitions that are not used. Removing unused code helps maintain readability and avoids confusion.
Apply this diff to remove the unnecessary commented-out code:
-# dev-notes: never reach here, text never be nil, "" -# def handle({:command, :note, %{chat: chat, text: nil}}, _context) do -# end
Line range hint
475-484: Add error handling forExGram.send_phototo prevent potential crashesCurrently, the code assumes
ExGram.send_photoalways succeeds. If it fails, aMatchErrorwill occur due to pattern matching. It's better to handle both success and error cases.Apply this diff to include error handling:
-case {:ok, msg} = ExGram.send_photo(chat_id, content) +case ExGram.send_photo(chat_id, content) do + {:ok, msg} -> + file_id = get_file_id(msg) + # Rest of the code + image_base64 = + case file_content do + {:file, file} -> File.read!(file) |> Base.encode64() + {:file_content, file_content, _file_name} -> Base.encode64(file_content) + end + + PhotoService.create_photo!(%{ + image: image_base64, + caption: file_name, + file_id: file_id, + belongs_to_id: chat_id + }) + + {:error, reason} -> + Logger.error("Failed to send photo: #{inspect(reason)}") + send_message(chat_id, "Failed to send photo.") +end
121-124: Consistent handling of niltextin command handlersWhile handling the
/searchcommand withtext: nil, ensure that similar commands, like/note, also consistently handle cases wheretextmight benil.Consider adding a clause for
/noteto handletext: nilor ensure thattextis always present when these handlers are invoked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- .gitignore (1 hunks)
- README.md (2 hunks)
- config/runtime.exs (0 hunks)
- docs/dev-logs/2024-10-25.md (1 hunks)
- docs/dev/typesense.md (1 hunks)
- lib/migration/typesense.ex (1 hunks)
- lib/migration/typesense/note.ex (1 hunks)
- lib/migration/typesense/photo.ex (1 hunks)
- lib/save_it/bot.ex (12 hunks)
- lib/save_it/note_service.ex (1 hunks)
- lib/save_it/photo_service.ex (4 hunks)
- lib/small_sdk/typesense.ex (1 hunks)
- lib/small_sdk/typesense_admin.ex (0 hunks)
- priv/typesense/2024-10-24_create_photos_collection.exs (1 hunks)
- priv/typesense/2024-10-25_create_notes_collections.exs (1 hunks)
- priv/typesense/reset.exs (1 hunks)
- zeabur/template.yaml (1 hunks)
💤 Files with no reviewable changes (2)
- config/runtime.exs
- lib/small_sdk/typesense_admin.ex
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~71-~71: You might be missing the article “a” here.
Context: ...Pro Tips: create shell script for fast run app 1. touch start.shsh #!/bi...(AI_EN_LECTOR_MISSING_DETERMINER_A)
🪛 Markdownlint
docs/dev-logs/2024-10-25.md
3-3: Punctuation: '.'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
docs/dev/typesense.md
6-6: Column: 6
Hard tabs(MD010, no-hard-tabs)
7-7: Column: 6
Hard tabs(MD010, no-hard-tabs)
8-8: Column: 6
Hard tabs(MD010, no-hard-tabs)
9-9: Column: 6
Hard tabs(MD010, no-hard-tabs)
10-10: Column: 6
Hard tabs(MD010, no-hard-tabs)
11-11: Column: 6
Hard tabs(MD010, no-hard-tabs)
14-14: null
Bare URL used(MD034, no-bare-urls)
5-5: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
🔇 Additional comments (12)
priv/typesense/2024-10-25_create_notes_collections.exs (1)
1-1: 🛠️ Refactor suggestionConsider enhancing the migration script's robustness.
While the script is concise, consider adding error handling and logging for better maintainability and debugging:
Here's a suggested improvement:
-Migration.Typesense.Note.create_collection!() +try do + IO.puts("Creating notes collection in Typesense...") + + # Check if collection already exists to make the migration idempotent + unless Migration.Typesense.Note.collection_exists?() do + Migration.Typesense.Note.create_collection!() + IO.puts("✓ Notes collection created successfully") + else + IO.puts("→ Notes collection already exists, skipping creation") + end +rescue + error -> + IO.puts("✗ Failed to create notes collection: #{inspect(error)}") + reraise error, __STACKTRACE__ +endThis enhancement provides:
- Error visibility through logging
- Idempotency by checking for existing collection
- Clear success/failure indicators
- Proper error propagation while maintaining the crash-on-failure behavior
Let's verify if the suggested
collection_exists?function is already implemented:✅ Verification successful
Revise the suggested approach for collection creation
After reviewing the codebase, I need to modify my previous suggestion. The
Migration.Typesensemodule already handles API errors appropriately through thehandle_responsefunction, which raises exceptions for various error cases including 409 (Conflict) when a collection already exists. Therefore, the idempotency check isn't necessary, and we should focus on logging for better visibility:-Migration.Typesense.Note.create_collection!() +try do + IO.puts("Creating notes collection in Typesense...") + Migration.Typesense.Note.create_collection!() + IO.puts("✓ Notes collection created successfully") +rescue + error -> + IO.puts("✗ Failed to create notes collection: #{inspect(error)}") + reraise error, __STACKTRACE__ +end
- The error handling in
SmallSdk.Typesensealready covers all API error cases- The
create_collection!function will raise appropriate exceptions that we can catch and log- No need for explicit existence check as 409 Conflict will be raised if collection exists
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for collection_exists? function definition ast-grep --pattern 'def collection_exists?($$$) { $$$ }'Length of output: 58
Script:
#!/bin/bash # Let's try searching for collection_exists? function in Elixir syntax ast-grep --pattern 'def collection_exists? do $$$ end' # Also search for any mentions of collection_exists? in the codebase rg "collection_exists?" -A 3Length of output: 89
Script:
#!/bin/bash # Let's check the Migration.Typesense.Note module implementation fd "note.ex" --exec cat {} # Also check for any Typesense-related modules fd "typesense" --type f --exec cat {}Length of output: 5002
priv/typesense/2024-10-24_create_photos_collection.exs (1)
1-1: Verify collection schema and indexes.Since this migration creates a new collection, we should verify that the schema and indexes are correctly defined.
Let's check the collection schema definition:
✅ Verification successful
Let me gather more information about the schema definitions since they weren't visible in the previous results.
Schema and indexes for photos collection are well-defined
The photos collection schema is properly configured with:
- Image field with CLIP embedding support via
ts/clip-vit-b-p32model- Essential fields: caption (optional), file_id, belongs_to_id
- Proper sorting field (inserted_at)
- Appropriate data types and indexing settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find and inspect the collection schema definition # Look for the schema definition in the Photo module ast-grep --pattern 'def create_collection! do $$$ end' # Look for any other Typesense collection definitions for comparison rg -A 10 'create_collection!'Length of output: 2131
Script:
#!/bin/bash # Look for schema definitions in both Photo and Note modules rg -A 20 '@photos_schema' lib/migration/typesense/photo.ex rg -A 20 '@notes_schema' lib/migration/typesense/note.exLength of output: 1873
.gitignore (2)
29-31: LGTM! Good separation of local and shared data directories.The distinction between
_localanddatadirectories provides a clear separation between local development data and shared project data.
33-38: LGTM! Well-structured environment-specific script ignores.Good organization of environment-specific scripts with clear naming patterns:
_local*for local development_dev*for development_stag*for staging_prod*for productionnohup.outfor background process logsThis structure aligns well with modern deployment practices and helps prevent accidental commits of environment-specific scripts.
lib/migration/typesense/note.ex (2)
1-2: LGTM: Module structure follows conventionsThe module name and alias declaration are well-structured and follow Elixir naming conventions.
4-20: Verify schema design choicesThe schema design raises several points that need verification:
- The decision to store
file_iddirectly instead of using references- The relationship between
message_idandfile_id- The purpose and scope of
belongs_to_id- The use of
int64for timestamps instead of ISO8601 stringsLet's check how these fields are used in the codebase:
✅ Verification successful
Let me gather more specific information about how these fields are used.
Based on the gathered information, I can now provide a final response about the schema design choices.
Schema design is appropriate for the use case
The schema design choices are well-justified based on the codebase analysis:
file_idandmessage_idare correctly used together:
file_idstores Telegram's photo file identifiermessage_idis used to link notes to specific Telegram messages for editing/updating
belongs_to_idis consistently used to store the chat ID, enabling proper data segregation per chat/userUsing
int64for timestamps is consistent with the photos schema and appropriate for Unix timestampsThe schema effectively supports the application's core functionality as a Telegram bot that manages photos and their associated notes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage patterns of these fields echo "Searching for field usage patterns..." rg -l "file_id|message_id|belongs_to_id" --type elixir # Check timestamp handling in other Typesense collections echo "Checking timestamp handling in other collections..." ast-grep --pattern 'inserted_at|updated_at.*"type".*"int64"'Length of output: 458
Script:
#!/bin/bash # Check how file_id and message_id are used together echo "Checking file_id and message_id usage..." rg -B2 -A2 "file_id|message_id" lib/save_it/note_service.ex lib/save_it/bot.ex # Check belongs_to_id usage echo "Checking belongs_to_id usage..." rg -B2 -A2 "belongs_to_id" lib/save_it/note_service.ex # Check timestamp handling in photo schema echo "Checking photo schema timestamp handling..." cat lib/migration/typesense/photo.exLength of output: 11511
lib/migration/typesense/photo.ex (2)
7-9: Verify security implications of image storage.While the image data isn't stored directly (store: false), consider:
- Size limits for base64 encoded images
- Potential security risks with image metadata
- Access control for the collection
Let's check for other image-related security configurations:
#!/bin/bash # Search for image-related configurations rg -g '*.{ex,exs}' -i '(image.*size|content.*type|file.*limit)'
4-25: Add documentation and consider search optimization.The schema would benefit from:
- Module documentation explaining the purpose and structure
- Field-level documentation, especially for
belongs_to_id- Search optimization configurations
Add documentation and consider these improvements:
+ @moduledoc """ + Defines and manages the Typesense collection schema for photos. + This collection stores photo data with CLIP embeddings for similarity search. + """ + @doc """ + Schema for the photos collection. + - image: Base64 encoded image data (not stored) + - image_embedding: CLIP model embeddings for similarity search + - caption: Optional text description of the image + - file_id: Unique identifier for the file + - belongs_to_id: Reference to the parent resource + - inserted_at: Timestamp in Unix epoch format + """ @photos_schema %{Let's check if similar schemas exist in the codebase:
lib/save_it/note_service.ex (1)
1-5: LGTM: Module structure follows best practicesThe module structure is clean and follows Elixir conventions with appropriate Logger requirement and clear aliasing.
lib/save_it/photo_service.ex (2)
22-22: Consider error handling strategy with bang function.The change to
update_document!means errors will be raised instead of being returned. Ensure this aligns with the application's error handling strategy and that errors are properly caught at the appropriate level.Let's check the error handling pattern in the codebase:
#!/bin/bash # Look for try/rescue blocks that might handle these errors ast-grep --pattern 'try do $$$ rescue $$$ end'
1-1: Verify module rename impact across the codebase.The module rename from
TypesensePhototoPhotoServiceis a breaking change.Let's verify all references have been updated:
✅ Verification successful
Module rename has been properly propagated across the codebase
All references to the module have been updated to use
PhotoService. The search results show:
- No occurrences of the old name
TypesensePhoto- Consistent usage of
PhotoServiceinlib/save_it/bot.exwith proper aliasing- The module definition itself in
lib/save_it/photo_service.ex🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old module name rg "TypesensePhoto" --type elixir # Search for references to the new module name to confirm updates rg "PhotoService" --type elixirLength of output: 774
zeabur/template.yaml (1)
76-78: Review API key exposure configuration.The TYPESENSE_API_KEY is marked as
expose: true. Verify if this exposure is necessary and ensure it's only accessible to authorized services.#!/bin/bash # Search for TYPESENSE_API_KEY usage in the codebase rg "TYPESENSE_API_KEY" -A 3
| Migration.Typesense.Photo.reset!() | ||
| Migration.Typesense.Note.reset!() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and logging.
The current implementation could benefit from:
- Error handling to ensure both reset operations succeed or fail together
- Logging to provide visibility into the reset process
Consider refactoring like this:
# mix run priv/typesense/reset.exs
-Migration.Typesense.Photo.reset!()
-Migration.Typesense.Note.reset!()
+try do
+ IO.puts("Starting Typesense collections reset...")
+
+ IO.puts("Resetting photos collection...")
+ Migration.Typesense.Photo.reset!()
+
+ IO.puts("Resetting notes collection...")
+ Migration.Typesense.Note.reset!()
+
+ IO.puts("Successfully reset all collections")
+rescue
+ error ->
+ IO.puts("Failed to reset collections: #{inspect(error)}")
+ exit({:shutdown, 1})
+end📝 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.
| Migration.Typesense.Photo.reset!() | |
| Migration.Typesense.Note.reset!() | |
| # mix run priv/typesense/reset.exs | |
| try do | |
| IO.puts("Starting Typesense collections reset...") | |
| IO.puts("Resetting photos collection...") | |
| Migration.Typesense.Photo.reset!() | |
| IO.puts("Resetting notes collection...") | |
| Migration.Typesense.Note.reset!() | |
| IO.puts("Successfully reset all collections") | |
| rescue | |
| error -> | |
| IO.puts("Failed to reset collections: #{inspect(error)}") | |
| exit({:shutdown, 1}) | |
| end |
💡 Codebase verification
Based on the findings, I can now provide a final response:
The reset script affects multiple active services in the application:
save_it/photo_service.ex- Handles photo-related operationssave_it/note_service.ex- Manages note operationssave_it/bot.ex- Bot integration that likely uses these collectionssave_it/cobalt_client.ex- Client that interacts with these services
Before running the reset script, ensure:
- All services are prepared for temporary data unavailability
- Coordinate with the bot service to handle potential disruptions
- Consider implementing a reindex strategy if the data needs to be preserved
🔗 Analysis chain
Verify the impact on dependent systems.
Since this script resets both collections, we should verify if there are any systems or features that depend on this data and might need to be notified or updated after the reset.
Let's check for any direct dependencies:
Let me gather more information about how these collections are being used in the application:
Let me check one more thing to see if there are any web endpoints or controllers using these collections:
Let me try one last search with corrected syntax:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to these collections in the codebase
echo "Searching for Photo collection usage..."
rg -l "Typesense.Photo" --type elixir
echo -e "\nSearching for Note collection usage..."
rg -l "Typesense.Note" --type elixir
Length of output: 481
Script:
#!/bin/bash
# Check the implementation details of these modules and their usage
echo "=== Photo Collection Implementation ==="
rg -A 10 "defmodule.*Photo" lib/migration/typesense/photo.ex
echo -e "\n=== Note Collection Implementation ==="
rg -A 10 "defmodule.*Note" lib/migration/typesense/note.ex
# Search for any API endpoints or services using these collections
echo -e "\n=== Searching for API/Service Usage ==="
rg "Typesense\.(Photo|Note)" --type elixir -C 3 lib/
Length of output: 1543
Script:
#!/bin/bash
# Search for any controllers or web-related usage
echo "=== Checking Web Controllers and Routes ==="
fd -e ex -e exs --type f -X rg -l "(photos?|notes?)" lib/.*/(controller|router|live)
# Check for any background jobs or scheduled tasks
echo -e "\n=== Checking for Background Jobs ==="
fd -e ex --type f -X rg -l "(photos?|notes?)" lib/.*/job
Length of output: 318
Script:
#!/bin/bash
# Search for any controllers or web-related usage
echo "=== Checking Web Controllers and Routes ==="
rg -l "(photos?|notes?)" lib/ --type elixir
# Check migration files to understand the collection setup
echo -e "\n=== Checking Migration Files ==="
rg -C 3 "collection" priv/typesense/2024-10-*
Length of output: 757
| def create_collection!(schema) do | ||
| req = build_request("/collections") | ||
| {:ok, res} = Req.post(req, json: schema) | ||
|
|
||
| res.body | ||
| end |
There was a problem hiding this comment.
Add proper error handling and documentation for public functions.
The public API functions have several areas for improvement:
- Error handling: The functions assume requests will succeed (
{:ok, res}), but network issues or API errors could occur. - Input validation: Parameters should be validated before making requests.
- Documentation: Public functions should have
@docand@specannotations. - Response consistency: Consider wrapping response bodies in a consistent structure.
Here's a suggested improvement:
+ @doc """
+ Creates a new collection in Typesense.
+
+ ## Parameters
+ - schema: Map containing the collection schema
+
+ ## Returns
+ - {:ok, response} on success
+ - {:error, reason} on failure
+ """
+ @spec create_collection!(map()) :: {:ok, map()} | {:error, any()}
def create_collection!(schema) do
+ with {:ok, schema} <- validate_schema(schema),
+ req <- build_request("/collections"),
+ {:ok, res} <- Req.post(req, json: schema) do
+ {:ok, res.body}
+ else
+ {:error, reason} -> {:error, reason}
+ error -> {:error, error}
+ end
end
+ @doc """
+ Deletes a collection from Typesense.
+
+ ## Parameters
+ - collection_name: String name of the collection
+
+ ## Returns
+ - {:ok, response} on success
+ - {:error, reason} on failure
+ """
+ @spec delete_collection!(String.t()) :: {:ok, map()} | {:error, any()}
def delete_collection!(collection_name) do
+ with {:ok, name} <- validate_collection_name(collection_name),
+ req <- build_request("/collections/#{name}"),
+ {:ok, res} <- Req.delete(req) do
+ {:ok, res.body}
+ else
+ {:error, reason} -> {:error, reason}
+ error -> {:error, error}
+ end
endAdd these private validation functions:
defp validate_schema(%{} = schema) do
{:ok, schema}
end
defp validate_schema(_), do: {:error, "Invalid schema format"}
defp validate_collection_name(name) when is_binary(name) and byte_size(name) > 0 do
{:ok, name}
end
defp validate_collection_name(_), do: {:error, "Invalid collection name"}Also applies to: 9-14, 16-21
| defp get_env() do | ||
| url = Application.fetch_env!(:save_it, :typesense_url) | ||
| api_key = Application.fetch_env!(:save_it, :typesense_api_key) | ||
|
|
||
| {url, api_key} | ||
| end |
There was a problem hiding this comment.
Consider safer environment variable handling.
The get_env/0 function uses fetch_env! which raises an error if the configuration is missing. This could cause runtime crashes in production.
Consider using Application.get_env/3 with fallbacks or environment-specific validation:
defp get_env() do
- url = Application.fetch_env!(:save_it, :typesense_url)
- api_key = Application.fetch_env!(:save_it, :typesense_api_key)
-
- {url, api_key}
+ with {:ok, url} <- get_config(:typesense_url),
+ {:ok, api_key} <- get_config(:typesense_api_key) do
+ {:ok, {url, api_key}}
+ end
end
+ defp get_config(key) do
+ case Application.get_env(:save_it, key) do
+ nil -> {:error, "Missing configuration: #{key}"}
+ value when byte_size(value) > 0 -> {:ok, value}
+ _ -> {:error, "Invalid configuration: #{key}"}
+ end
+ endCommittable suggestion was skipped due to low confidence.
| defp build_request(path) do | ||
| {url, api_key} = get_env() | ||
|
|
||
| Req.new( | ||
| base_url: url, | ||
| url: path, | ||
| headers: [ | ||
| {"Content-Type", "application/json"}, | ||
| {"X-TYPESENSE-API-KEY", api_key} | ||
| ] | ||
| ) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add request timeout configuration and consider making headers configurable.
The request configuration could be improved with timeout settings and more flexible header configuration.
Consider these improvements:
defp build_request(path) do
- {url, api_key} = get_env()
+ with {:ok, {url, api_key}} <- get_env() do
+ Req.new(
+ base_url: url,
+ url: path,
+ headers: build_headers(api_key),
+ connect_options: [timeout: get_timeout()],
+ retry: :transient
+ )
+ end
end
+ defp build_headers(api_key) do
+ [
+ {"Content-Type", "application/json"},
+ {"X-TYPESENSE-API-KEY", api_key}
+ ]
+ end
+ defp get_timeout do
+ Application.get_env(:save_it, :typesense_timeout, 5000)
+ end📝 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.
| defp build_request(path) do | |
| {url, api_key} = get_env() | |
| Req.new( | |
| base_url: url, | |
| url: path, | |
| headers: [ | |
| {"Content-Type", "application/json"}, | |
| {"X-TYPESENSE-API-KEY", api_key} | |
| ] | |
| ) | |
| end | |
| defp build_request(path) do | |
| with {:ok, {url, api_key}} <- get_env() do | |
| Req.new( | |
| base_url: url, | |
| url: path, | |
| headers: build_headers(api_key), | |
| connect_options: [timeout: get_timeout()], | |
| retry: :transient | |
| ) | |
| end | |
| end | |
| defp build_headers(api_key) do | |
| [ | |
| {"Content-Type", "application/json"}, | |
| {"X-TYPESENSE-API-KEY", api_key} | |
| ] | |
| end | |
| defp get_timeout do | |
| Application.get_env(:save_it, :typesense_timeout, 5000) | |
| end |
| def create_collection!() do | ||
| Typesense.create_collection!(@photos_schema) | ||
| end | ||
|
|
||
| def reset!() do | ||
| Typesense.delete_collection!(@photos_schema["name"]) | ||
| Typesense.create_collection!(@photos_schema) | ||
| end |
There was a problem hiding this comment.
Add documentation and error handling.
The public functions need:
@docand@specannotations- Error handling for Typesense operations
- Return value documentation
Consider this improvement:
+ @doc """
+ Creates a new photos collection in Typesense.
+
+ Returns `:ok` on success, or raises an error if the collection already exists
+ or if there's a connection issue.
+ """
+ @spec create_collection!() :: :ok | no_return()
def create_collection!() do
- Typesense.create_collection!(@photos_schema)
+ case Typesense.create_collection!(@photos_schema) do
+ {:ok, _} -> :ok
+ {:error, %{"message" => message}} -> raise "Failed to create collection: #{message}"
+ end
end
+ @doc """
+ Resets the photos collection by deleting and recreating it.
+
+ Returns `:ok` on success, or raises an error if the operations fail.
+ """
+ @spec reset!() :: :ok | no_return()
def reset!() do
- Typesense.delete_collection!(@photos_schema["name"])
- Typesense.create_collection!(@photos_schema)
+ with :ok <- Typesense.delete_collection!(@photos_schema["name"]),
+ :ok <- create_collection!() do
+ :ok
+ end
end📝 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.
| def create_collection!() do | |
| Typesense.create_collection!(@photos_schema) | |
| end | |
| def reset!() do | |
| Typesense.delete_collection!(@photos_schema["name"]) | |
| Typesense.create_collection!(@photos_schema) | |
| end | |
| @doc """ | |
| Creates a new photos collection in Typesense. | |
| Returns `:ok` on success, or raises an error if the collection already exists | |
| or if there's a connection issue. | |
| """ | |
| @spec create_collection!() :: :ok | no_return() | |
| def create_collection!() do | |
| case Typesense.create_collection!(@photos_schema) do | |
| {:ok, _} -> :ok | |
| {:error, %{"message" => message}} -> raise "Failed to create collection: #{message}" | |
| end | |
| end | |
| @doc """ | |
| Resets the photos collection by deleting and recreating it. | |
| Returns `:ok` on success, or raises an error if the operations fail. | |
| """ | |
| @spec reset!() :: :ok | no_return() | |
| def reset!() do | |
| with :ok <- Typesense.delete_collection!(@photos_schema["name"]), | |
| :ok <- create_collection!() do | |
| :ok | |
| end | |
| end |
| def search_documents!(collection_name, opts) do | ||
| q = Keyword.get(opts, :q, "*") | ||
| query_by = Keyword.get(opts, :query_by, "") | ||
| filter_by = Keyword.get(opts, :filter_by, "") | ||
|
|
||
| req = build_request("/collections/#{collection_name}/documents/search") | ||
|
|
||
| {:ok, res} = | ||
| Req.get(req, | ||
| params: %{ | ||
| q: q, | ||
| query_by: query_by, | ||
| filter_by: filter_by | ||
| } | ||
| ) | ||
|
|
||
| data = handle_response(res) | ||
|
|
||
| data["hits"] |> Enum.map(&Map.get(&1, "document")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve search_documents!/2 implementation.
The new search function needs several improvements:
+ @type search_opts :: [
+ q: String.t(),
+ query_by: String.t(),
+ filter_by: String.t()
+ ]
+
+ @doc """
+ Searches for documents in the specified Typesense collection.
+
+ ## Parameters
+ - collection_name: The name of the collection
+ - opts: Keyword list of search options
+ - :q - Search query (default: "*")
+ - :query_by - Fields to query
+ - :filter_by - Filter expression
+
+ ## Returns
+ List of matching documents
+
+ ## Examples
+ iex> search_documents!("photos", q: "vacation", query_by: "title,tags")
+ [%{"id" => "1", "title" => "Beach vacation"}, ...]
+ """
+ @spec search_documents!(String.t(), search_opts()) :: [map()]
def search_documents!(collection_name, opts) do
+ unless is_binary(collection_name) and collection_name != "",
+ do: raise ArgumentError, "collection_name must be a non-empty string"
+
q = Keyword.get(opts, :q, "*")
query_by = Keyword.get(opts, :query_by, "")
filter_by = Keyword.get(opts, :filter_by, "")
req = build_request("/collections/#{collection_name}/documents/search")
{:ok, res} =
Req.get(req,
params: %{
q: q,
query_by: query_by,
filter_by: filter_by
+ # Consider adding other Typesense search parameters:
+ # page, per_page, sort_by, prefix, etc.
}
)
data = handle_response(res)
- data["hits"] |> Enum.map(&Map.get(&1, "document"))
+ get_in(data, ["hits", Access.all(), "document"]) || []
end📝 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.
| def search_documents!(collection_name, opts) do | |
| q = Keyword.get(opts, :q, "*") | |
| query_by = Keyword.get(opts, :query_by, "") | |
| filter_by = Keyword.get(opts, :filter_by, "") | |
| req = build_request("/collections/#{collection_name}/documents/search") | |
| {:ok, res} = | |
| Req.get(req, | |
| params: %{ | |
| q: q, | |
| query_by: query_by, | |
| filter_by: filter_by | |
| } | |
| ) | |
| data = handle_response(res) | |
| data["hits"] |> Enum.map(&Map.get(&1, "document")) | |
| @type search_opts :: [ | |
| q: String.t(), | |
| query_by: String.t(), | |
| filter_by: String.t() | |
| ] | |
| @doc """ | |
| Searches for documents in the specified Typesense collection. | |
| ## Parameters | |
| - collection_name: The name of the collection | |
| - opts: Keyword list of search options | |
| - :q - Search query (default: "*") | |
| - :query_by - Fields to query | |
| - :filter_by - Filter expression | |
| ## Returns | |
| List of matching documents | |
| ## Examples | |
| iex> search_documents!("photos", q: "vacation", query_by: "title,tags") | |
| [%{"id" => "1", "title" => "Beach vacation"}, ...] | |
| """ | |
| @spec search_documents!(String.t(), search_opts()) :: [map()] | |
| def search_documents!(collection_name, opts) do | |
| unless is_binary(collection_name) and collection_name != "", | |
| do: raise ArgumentError, "collection_name must be a non-empty string" | |
| q = Keyword.get(opts, :q, "*") | |
| query_by = Keyword.get(opts, :query_by, "") | |
| filter_by = Keyword.get(opts, :filter_by, "") | |
| req = build_request("/collections/#{collection_name}/documents/search") | |
| {:ok, res} = | |
| Req.get(req, | |
| params: %{ | |
| q: q, | |
| query_by: query_by, | |
| filter_by: filter_by | |
| # Consider adding other Typesense search parameters: | |
| # page, per_page, sort_by, prefix, etc. | |
| } | |
| ) | |
| data = handle_response(res) | |
| get_in(data, ["hits", Access.all(), "document"]) || [] | |
| end |
| def handle_response(res) do | ||
| case res do | ||
| %Req.Response{status: 200} -> | ||
| res.body | ||
|
|
||
| %Req.Response{status: 201} -> | ||
| res.body | ||
|
|
||
| %Req.Response{status: 400} -> | ||
| Logger.error("Bad Request: #{inspect(res.body)}") | ||
| raise "Bad Request" | ||
|
|
||
| %Req.Response{status: 401} -> | ||
| raise "Unauthorized" | ||
|
|
||
| %Req.Response{status: 404} -> | ||
| nil | ||
|
|
||
| %Req.Response{status: 409} -> | ||
| raise "Conflict" | ||
|
|
||
| %Req.Response{status: 422} -> | ||
| raise "Unprocessable Entity" | ||
|
|
||
| %Req.Response{status: 503} -> | ||
| raise "Service Unavailable" | ||
|
|
||
| _ -> | ||
| raise "Unknown error" | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and improve error handling.
The new centralized error handling is a good addition, but could be improved in several ways:
- Add
@docand@specdocumentation - Include more context in error messages
- Handle network-related errors
- Consider consistent error handling strategy (currently mixes nil and raises)
+ @doc """
+ Handles HTTP responses from Typesense API.
+
+ ## Examples
+
+ iex> handle_response(%Req.Response{status: 200, body: %{"key" => "value"}})
+ %{"key" => "value"}
+
+ iex> handle_response(%Req.Response{status: 404})
+ nil
+ """
+ @spec handle_response(Req.Response.t()) :: map() | nil | no_return()
def handle_response(res) do
case res do
%Req.Response{status: 200} ->
res.body
%Req.Response{status: 201} ->
res.body
%Req.Response{status: 400} ->
- Logger.error("Bad Request: #{inspect(res.body)}")
- raise "Bad Request"
+ Logger.error("Typesense Bad Request: #{inspect(res.body)}")
+ raise "Typesense Bad Request: #{inspect(res.body)}"
%Req.Response{status: 401} ->
- raise "Unauthorized"
+ raise "Typesense Unauthorized: #{inspect(res.body)}"
%Req.Response{status: 404} ->
nil
%Req.Response{status: 409} ->
- raise "Conflict"
+ raise "Typesense Conflict: #{inspect(res.body)}"
%Req.Response{status: 422} ->
- raise "Unprocessable Entity"
+ raise "Typesense Unprocessable Entity: #{inspect(res.body)}"
%Req.Response{status: 503} ->
- raise "Service Unavailable"
+ raise "Typesense Service Unavailable: #{inspect(res.body)}"
_ ->
- raise "Unknown error"
+ raise "Unknown Typesense error: #{inspect(res)}"
end
end📝 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.
| def handle_response(res) do | |
| case res do | |
| %Req.Response{status: 200} -> | |
| res.body | |
| %Req.Response{status: 201} -> | |
| res.body | |
| %Req.Response{status: 400} -> | |
| Logger.error("Bad Request: #{inspect(res.body)}") | |
| raise "Bad Request" | |
| %Req.Response{status: 401} -> | |
| raise "Unauthorized" | |
| %Req.Response{status: 404} -> | |
| nil | |
| %Req.Response{status: 409} -> | |
| raise "Conflict" | |
| %Req.Response{status: 422} -> | |
| raise "Unprocessable Entity" | |
| %Req.Response{status: 503} -> | |
| raise "Service Unavailable" | |
| _ -> | |
| raise "Unknown error" | |
| end | |
| end | |
| @doc """ | |
| Handles HTTP responses from Typesense API. | |
| ## Examples | |
| iex> handle_response(%Req.Response{status: 200, body: %{"key" => "value"}}) | |
| %{"key" => "value"} | |
| iex> handle_response(%Req.Response{status: 404}) | |
| nil | |
| """ | |
| @spec handle_response(Req.Response.t()) :: map() | nil | no_return() | |
| def handle_response(res) do | |
| case res do | |
| %Req.Response{status: 200} -> | |
| res.body | |
| %Req.Response{status: 201} -> | |
| res.body | |
| %Req.Response{status: 400} -> | |
| Logger.error("Typesense Bad Request: #{inspect(res.body)}") | |
| raise "Typesense Bad Request: #{inspect(res.body)}" | |
| %Req.Response{status: 401} -> | |
| raise "Typesense Unauthorized: #{inspect(res.body)}" | |
| %Req.Response{status: 404} -> | |
| nil | |
| %Req.Response{status: 409} -> | |
| raise "Typesense Conflict: #{inspect(res.body)}" | |
| %Req.Response{status: 422} -> | |
| raise "Typesense Unprocessable Entity: #{inspect(res.body)}" | |
| %Req.Response{status: 503} -> | |
| raise "Typesense Service Unavailable: #{inspect(res.body)}" | |
| _ -> | |
| raise "Unknown Typesense error: #{inspect(res)}" | |
| end | |
| end |
| media: photo["file_id"], | ||
| caption: "Found photos", | ||
| show_caption_above_media: true | ||
| } |
There was a problem hiding this comment.
Remove invalid parameter show_caption_above_media from InputMediaPhoto
The InputMediaPhoto struct does not support the show_caption_above_media parameter. Including it may result in an error when sending media groups.
Apply this diff to correct the struct:
%ExGram.Model.InputMediaPhoto{
type: "photo",
media: photo["file_id"],
caption: "Found photos",
- show_caption_above_media: true
}📝 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.
| media: photo["file_id"], | |
| caption: "Found photos", | |
| show_caption_above_media: true | |
| } | |
| media: photo["file_id"], | |
| caption: "Found photos" | |
| } |
| def handle({:edited_message, msg}, _context) do | ||
| %{message_id: message_id, chat: chat, text: text} = msg | ||
|
|
||
| edited_note_text = | ||
| case Regex.run(~r/\/note\s+(.*)/, text) do | ||
| [_, edited_note_text] -> edited_note_text | ||
| _ -> nil | ||
| end | ||
|
|
||
| case String.contains?(text, "/note") do | ||
| true -> | ||
| note = NoteService.get_note!(message_id, chat.id) | ||
|
|
||
| NoteService.update_note!(note["id"], %{"content" => edited_note_text}) | ||
|
|
||
| false -> | ||
| Logger.info("edited message: #{inspect(text)}, ignore it") | ||
| end |
There was a problem hiding this comment.
Ensure text is not nil before processing edited_message
When handling edited messages, if msg.text is nil, using Regex.run and String.contains? will raise an exception. It's essential to check if text is present before performing these operations.
Apply this diff to handle cases where text is nil:
def handle({:edited_message, msg}, _context) do
%{message_id: message_id, chat: chat, text: text} = msg
+ if text == nil do
+ Logger.info("Edited message has no text; ignoring.")
+ {:ok, nil}
+ else
edited_note_text =
case Regex.run(~r/\/note\s+(.*)/, text) do
[_, edited_note_text] -> edited_note_text
_ -> nil
end
case String.contains?(text, "/note") do
true ->
note = NoteService.get_note!(message_id, chat.id)
NoteService.update_note!(note["id"], %{"content" => edited_note_text})
false ->
Logger.info("Edited message: #{inspect(text)}, ignoring.")
end
+ end
end📝 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.
| def handle({:edited_message, msg}, _context) do | |
| %{message_id: message_id, chat: chat, text: text} = msg | |
| edited_note_text = | |
| case Regex.run(~r/\/note\s+(.*)/, text) do | |
| [_, edited_note_text] -> edited_note_text | |
| _ -> nil | |
| end | |
| case String.contains?(text, "/note") do | |
| true -> | |
| note = NoteService.get_note!(message_id, chat.id) | |
| NoteService.update_note!(note["id"], %{"content" => edited_note_text}) | |
| false -> | |
| Logger.info("edited message: #{inspect(text)}, ignore it") | |
| end | |
| def handle({:edited_message, msg}, _context) do | |
| %{message_id: message_id, chat: chat, text: text} = msg | |
| if text == nil do | |
| Logger.info("Edited message has no text; ignoring.") | |
| {:ok, nil} | |
| else | |
| edited_note_text = | |
| case Regex.run(~r/\/note\s+(.*)/, text) do | |
| [_, edited_note_text] -> edited_note_text | |
| _ -> nil | |
| end | |
| case String.contains?(text, "/note") do | |
| true -> | |
| note = NoteService.get_note!(message_id, chat.id) | |
| NoteService.update_note!(note["id"], %{"content" => edited_note_text}) | |
| false -> | |
| Logger.info("Edited message: #{inspect(text)}, ignoring.") | |
| end | |
| end | |
| end |
| def handle( | ||
| {:command, :note, | ||
| %{message_id: message_id, chat: chat, text: text, reply_to_message: reply_to_message}}, | ||
| _context | ||
| ) | ||
| when is_binary(text) do | ||
| file_id = reply_to_message.photo |> List.last() |> Map.get(:file_id) | ||
|
|
||
| case file_id do | ||
| nil -> | ||
| send_message(chat.id, "Please reply to a photo to add a note.") | ||
|
|
||
| _ -> | ||
| case String.trim(text) do | ||
| "" -> | ||
| send_message(chat.id, "What note do you want to add?") | ||
|
|
||
| note_content -> | ||
| note = | ||
| NoteService.create_note!(%{ | ||
| content: note_content, | ||
| message_id: message_id, | ||
| file_id: file_id, | ||
| belongs_to_id: chat.id | ||
| }) | ||
|
|
||
| case note do | ||
| nil -> send_message(chat.id, "Failed to add note.") | ||
| # TODO:nice_to_have: 添加一个 emoji 即可 | ||
| _ -> send_message(chat.id, "Note added successfully.") | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Handle potential nil reply_to_message to prevent crashes
In the :note command handler, accessing reply_to_message.photo without checking if reply_to_message or reply_to_message.photo is nil can lead to exceptions if the command is not a reply to a photo message. It's important to validate these variables before use.
Apply this diff to add necessary checks:
def handle(
{:command, :note,
%{message_id: message_id, chat: chat, text: text, reply_to_message: reply_to_message}},
_context
)
when is_binary(text) do
+ if reply_to_message == nil do
+ send_message(chat.id, "Please reply to a photo to add a note.")
+ else
+ file_id = reply_to_message.photo |> List.last() |> Map.get(:file_id)
+
+ if file_id == nil do
+ send_message(chat.id, "Please reply to a photo to add a note.")
+ else
case String.trim(text) do
"" ->
send_message(chat.id, "What note do you want to add?")
note_content ->
note =
NoteService.create_note!(%{
content: note_content,
message_id: message_id,
file_id: file_id,
belongs_to_id: chat.id
})
case note do
nil -> send_message(chat.id, "Failed to add note.")
_ -> send_message(chat.id, "Note added successfully.")
end
end
+ end
end
end📝 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.
| def handle( | |
| {:command, :note, | |
| %{message_id: message_id, chat: chat, text: text, reply_to_message: reply_to_message}}, | |
| _context | |
| ) | |
| when is_binary(text) do | |
| file_id = reply_to_message.photo |> List.last() |> Map.get(:file_id) | |
| case file_id do | |
| nil -> | |
| send_message(chat.id, "Please reply to a photo to add a note.") | |
| _ -> | |
| case String.trim(text) do | |
| "" -> | |
| send_message(chat.id, "What note do you want to add?") | |
| note_content -> | |
| note = | |
| NoteService.create_note!(%{ | |
| content: note_content, | |
| message_id: message_id, | |
| file_id: file_id, | |
| belongs_to_id: chat.id | |
| }) | |
| case note do | |
| nil -> send_message(chat.id, "Failed to add note.") | |
| # TODO:nice_to_have: 添加一个 emoji 即可 | |
| _ -> send_message(chat.id, "Note added successfully.") | |
| end | |
| end | |
| end | |
| end | |
| def handle( | |
| {:command, :note, | |
| %{message_id: message_id, chat: chat, text: text, reply_to_message: reply_to_message}}, | |
| _context | |
| ) | |
| when is_binary(text) do | |
| if reply_to_message == nil do | |
| send_message(chat.id, "Please reply to a photo to add a note.") | |
| else | |
| file_id = reply_to_message.photo |> List.last() |> Map.get(:file_id) | |
| if file_id == nil do | |
| send_message(chat.id, "Please reply to a photo to add a note.") | |
| else | |
| case String.trim(text) do | |
| "" -> | |
| send_message(chat.id, "What note do you want to add?") | |
| note_content -> | |
| note = | |
| NoteService.create_note!(%{ | |
| content: note_content, | |
| message_id: message_id, | |
| file_id: file_id, | |
| belongs_to_id: chat.id | |
| }) | |
| case note do | |
| nil -> send_message(chat.id, "Failed to add note.") | |
| _ -> send_message(chat.id, "Note added successfully.") | |
| end | |
| end | |
| end | |
| end | |
| end |
|
放弃在 save_it bot 添加 notes 功能，保持 save_it 简单。 |
Summary by CodeRabbit
Release Notes
New Features
Documentation
README.mdto include a new "Features" section and expanded instructions for starting the Typesense service.Configuration