Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds per-user Telegram settings (auto-fetch in DMs): DB model, async DB engine/session, Alembic migrations and tooling, bot commands/handlers (/start, /settings, callback), auto-fetch DM flow, Docker/compose/env changes for persistent data, tests, and ancillary packaging/pyproject updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TelegramBot
participant Handlers as MessageHandler
participant UserSettings
participant Database
participant ItemProcessor
User->>TelegramBot: /settings
TelegramBot->>Handlers: settings_command
Handlers->>UserSettings: ensure_user_settings(user_id)
UserSettings->>Database: Query/Create UserSetting
Database-->>UserSettings: Return setting
UserSettings-->>Handlers: auto_fetch_in_dm value
Handlers->>TelegramBot: send settings message (keyboard)
User->>TelegramBot: press toggle
TelegramBot->>Handlers: settings_callback
Handlers->>UserSettings: toggle auto_fetch_in_dm
UserSettings->>Database: Update row
Database-->>UserSettings: Confirm
Handlers->>TelegramBot: update message with new state
sequenceDiagram
actor User
participant TelegramBot
participant DMHandler
participant UserSettings
participant Database
participant ItemProcessor
User->>TelegramBot: Send DM with URL(s)
TelegramBot->>DMHandler: route private message
DMHandler->>UserSettings: ensure_user_settings(user_id)
UserSettings->>Database: Query/Create row
DMHandler->>UserSettings: get_auto_fetch_in_dm(user_id)
UserSettings->>Database: Read preference
Database-->>UserSettings: Return preference
alt auto_fetch enabled
DMHandler->>ItemProcessor: _auto_fetch_urls(message)
ItemProcessor->>ItemProcessor: extract & fetch metadata
loop per URL
ItemProcessor->>Database: check rules / save item
ItemProcessor-->>TelegramBot: send item message
end
else auto_fetch disabled
DMHandler->>TelegramBot: show interactive buttons
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
packages/shared/fastfetchbot_shared/database/models/__init__.py (1)
1-1: Remove the unusednoqa: F401directive.Ruff (RUF100) reports that
F401is not enabled in the project's Ruff configuration, making the# noqa: F401comment inert. Either remove it, or enableF401inruff.toml/pyproject.tomlso it correctly suppresses the re-export warning.♻️ Proposed fix (if keeping F401 disabled)
-from fastfetchbot_shared.database.models.user_setting import UserSetting # noqa: F401 +from fastfetchbot_shared.database.models.user_setting import UserSetting🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/fastfetchbot_shared/database/models/__init__.py` at line 1, The line re-exporting UserSetting uses an unnecessary "# noqa: F401" comment because F401 is not enabled in Ruff; remove the unused directive from the import (the import statement "from fastfetchbot_shared.database.models.user_setting import UserSetting" in __init__.py) so the file no longer contains the inert "# noqa: F401" comment, or alternatively enable F401 in your project Ruff config if you intend to keep suppressing unused-import warnings.packages/shared/alembic/versions/0001_add_user_settings_table.py (1)
23-25: Prefersa.true()over the raw string"1"for the Boolean server defaultThe idiomatic SQLAlchemy form is
server_default=sa.sql.expression.true(), which rendersDEFAULT trueon PostgreSQL andDEFAULT 1on SQLite. Using the raw string"1"works on both backends today, but diverges from what Alembic autogenerate would emit if you ever regenerate the migration from the ORM model — potentially causing spurious "server default changed" detections.🛠️ Proposed fix
- sa.Column( - "auto_fetch_in_dm", sa.Boolean(), server_default="1", nullable=False - ), + sa.Column( + "auto_fetch_in_dm", sa.Boolean(), server_default=sa.true(), nullable=False + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/alembic/versions/0001_add_user_settings_table.py` around lines 23 - 25, The server_default for the sa.Column "auto_fetch_in_dm" should use SQLAlchemy's boolean expression instead of the raw string; update the Column definition in the migration (the sa.Column for "auto_fetch_in_dm") to use sa.sql.expression.true() (or sa.true()) as the server_default so Alembic/autogenerate will render DEFAULT true/1 consistently across backends.apps/telegram-bot/pyproject.toml (1)
6-6:[postgres]extra unconditionally installs asyncpg for all deploymentsThe
fastfetchbot-shared[postgres]extra was designed to be optional (it's a separate group inpackages/shared/pyproject.toml), yet it's hardcoded here, meaningasyncpgis always installed even for the default SQLite-only deployment. Consider keeping the base dependency and letting deployers opt in via an environment-specific extras mechanism or a separate[telegram-bot-postgres]optional-dependency entry in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/telegram-bot/pyproject.toml` at line 6, The dependency line currently unconditionally adds the postgres extra ("fastfetchbot-shared[postgres]") which forces asyncpg to be installed; change it to the plain package name ("fastfetchbot-shared") and add an opt-in extras entry for this project (e.g., define a telegram-bot-postgres optional dependency/extras group in this pyproject.toml) so deployers can opt into Postgres support, or document using environment-specific extras to install "fastfetchbot-shared[postgres]" when needed; update the dependency reference in apps/telegram-bot/pyproject.toml (the line containing "fastfetchbot-shared[postgres]") and add the corresponding optional extras section for the telegram bot.apps/telegram-bot/core/handlers/messages.py (1)
24-24: Suppress the RuffARG001false positive for the protocol-requiredcontextparameterThe
context: CallbackContextargument is mandated by python-telegram-bot's handler protocol. Add a# noqa: ARG001comment or rename to_contextto silence the Ruff warning without changing behaviour.🛠️ Proposed fix
-async def all_messages_process(update: Update, context: CallbackContext) -> None: +async def all_messages_process(update: Update, context: CallbackContext) -> None: # noqa: ARG001🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/telegram-bot/core/handlers/messages.py` at line 24, The Ruff ARG001 false positive is triggered by the protocol-required parameter context in the function all_messages_process; silence it by either renaming the unused parameter to _context or appending a lint suppression comment to the parameter like "context: CallbackContext # noqa: ARG001" so the handler signature required by python-telegram-bot is preserved while suppressing Ruff. Update the function definition in all_messages_process accordingly and ensure no other references to context remain or are renamed if you choose the underscore approach.packages/shared/pyproject.toml (1)
16-17:sqlalchemy[asyncio]andaiosqliteas core dependencies bloat all consumersEvery package that depends on
fastfetchbot-shared— including the API and any future service — will now pull in SQLAlchemy and aiosqlite, even if it never touches a database. The same principle used forasyncpg(optional[postgres]extra) andalembic(optional[migrate]extra) should apply here: expose a[database]or[sqlite]optional group, and only the telegram-bot (which actually uses the settings DB) adds it to its own dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/pyproject.toml` around lines 16 - 17, Move the heavy DB deps out of the base dependencies so consumers aren't forced to install them: remove "sqlalchemy[asyncio]>=2.0.0" and "aiosqlite>=0.17.0" from the main dependencies in fastfetchbot-shared's pyproject.toml and add them to a new optional extras group (e.g. [project.optional-dependencies] database = ["sqlalchemy[asyncio]>=2.0.0","aiosqlite>=0.17.0"] or use a name like "sqlite"); then update the consumer that needs the DB (telegram-bot) to declare its dependency on fastfetchbot-shared[database] (or [sqlite]) so only that package pulls in these libs.packages/shared/tests/test_user_setting.py (1)
74-115: Consider testing the actualensure_user_settingsservice function.This test re-implements the "ensure" logic inline instead of calling
ensure_user_settingsfromcore.services.user_settings. While it's valid as a model-level test, it won't catch regressions in the actual service function. Consider adding a separate integration test that exercises the real service, or at minimum note this as a model-only test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/tests/test_user_setting.py` around lines 74 - 115, The test currently re-implements the "ensure" behavior inline instead of exercising the actual service; update this test to call the real ensure_user_settings function from core.services.user_settings (or add a new integration test) rather than manually adding UserSetting rows: locate the test function test_ensure_user_settings_creates_row and replace the manual creation/commit logic that uses UserSetting(telegram_user_id=...) and direct db_session queries with calls to ensure_user_settings(db_session, telegram_user_id=user_id) (or call it twice to test create-then-no-op), then assert on the returned or fetched UserSetting.auto_fetch_in_dm and created_at to verify behavior and ensure regressions in ensure_user_settings are caught.packages/shared/alembic/env.py (1)
20-24:_DEFAULT_DATABASE_URLduplicated fromengine.py.The default URL
"sqlite+aiosqlite:///data/fastfetchbot.db"is defined in both this file andengine.py. If the default ever changes, it needs updating in two places. Consider importing or centralizing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/alembic/env.py` around lines 20 - 24, The default DB URL string is duplicated; replace the local _DEFAULT_DATABASE_URL in env.py by importing the single source of truth (e.g., the constant defined in engine.py or a new shared constant module) and update get_url() to return os.environ.get("SETTINGS_DATABASE_URL", <imported_constant>); remove the duplicate literal so changes only need to be made in one place and ensure the imported symbol name matches the one exported by engine.py (or create/export a shared DEFAULT_DATABASE_URL constant).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 153-159: The current .gitignore uses a directory ignore ".idea/"
which prevents any subsequent negations like "!.idea/runConfigurations/" from
taking effect; replace the directory ignore with a glob that ignores children
but not the directory node (change ".idea/" to ".idea/**") and then explicitly
re-include the desired paths and their contents by adding negations for both the
directory and its subtree (for example keep "!.idea/runConfigurations/" and add
"!.idea/runConfigurations/**", do the same for "!.idea/codeStyles/",
"!.idea/inspectionProfiles/", and ensure "!.idea/*.iml" remains to include
top-level .iml files).
In `@apps/telegram-bot/core/config.py`:
- Line 125: The config variable ITEM_DATABASE_ON is reading the wrong env key
because get_env_bool(env, "DATABASE_ON", False) still uses "DATABASE_ON"; update
the call to use the matching env key "ITEM_DATABASE_ON" (i.e., change the string
passed to get_env_bool) or, if the rename was only internal, add a clear inline
comment next to ITEM_DATABASE_ON explaining the env key intentionally remains
"DATABASE_ON"; if you change the env key, also update the docs/template.env and
add a migration note so deployers know to rename DATABASE_ON ->
ITEM_DATABASE_ON.
In `@apps/telegram-bot/core/handlers/commands.py`:
- Around line 58-77: Extract the DB toggle logic from the command handler into a
service function named toggle_auto_fetch_in_dm(user_id: int) -> bool in
core/services/user_settings.py: move the
select(UserSetting).where(UserSetting.telegram_user_id == user_id) lookup and
the create-or-toggle mutation (using get_session and UserSetting) into that
function, return the new auto_fetch_in_dm boolean, and then replace the inline
block in the handler with a single call to toggle_auto_fetch_in_dm(user_id)
assigning its return value to new_value; keep references to get_session,
UserSetting and the handler's user_id to locate the code.
- Around line 29-35: Replace the direct DB query in settings_command with the
existing service wrapper: call and await get_auto_fetch_in_dm(user_id) (from
core.services.user_settings) to obtain the auto_fetch value instead of opening a
session and executing select(UserSetting); remove the now-unneeded imports
(get_session, UserSetting, select) and ensure settings_command uses the service
function result for the toggle logic (or delegate toggle logic to a service
similarly) so DB access stays in the service layer.
In `@apps/telegram-bot/core/handlers/messages.py`:
- Around line 29-30: Wrap the await ensure_user_settings(message.from_user.id)
call in a try/except to catch DB/network errors so they don't bubble into the
global error handler; specifically, in the message handler where you call
ensure_user_settings, change it to: try: await ensure_user_settings(...) except
Exception as e: call the existing error logging function (e.g.,
error_process.error or logger.exception) with a concise message and the
exception, then continue without re-raising so private messages aren’t blocked
by transient DB failures.
- Around line 29-30: Concurrent calls to ensure_user_settings can race past the
_known_user_ids guard and both attempt to insert the same user, causing
IntegrityError; wrap the DB insert in ensure_user_settings around a try/except
that catches sqlalchemy.exc.IntegrityError, roll back the session on exception,
optionally reload the user to confirm existence, and then continue without
raising; alternatively replace the insert with an INSERT ... ON CONFLICT DO
NOTHING / INSERT ... ON CONFLICT IGNORE at the SQL level. Make sure to import
IntegrityError (from sqlalchemy.exc) and update any in-memory _known_user_ids
only after a successful insert or after confirming the record exists.
In `@apps/telegram-bot/core/services/user_settings.py`:
- Around line 11-21: ensure_user_settings can raise IntegrityError if two
coroutines concurrently insert the same new user: keep the existing cache check
and DB query in ensure_user_settings, but wrap the session add/commit of
UserSetting(telegram_user_id=user_id) in a try/except that catches
sqlalchemy.exc.IntegrityError, performs session.rollback() (or ensures the
transaction is aborted) and treats the error as a no-op; still add user_id to
_known_user_ids after the DB operation to avoid future races. Reference
ensure_user_settings, _known_user_ids, get_session, and UserSetting when making
the change.
In `@docker-compose.template.yml`:
- Around line 60-74: The template contains hardcoded weak defaults: the example
SETTINGS_DATABASE_URL and the commented postgres block use the literal password
"fastfetchbot"; update these to use a placeholder or env var to force
customization—replace the password in the example connection string and the
POSTGRES_PASSWORD value with a clear placeholder (e.g., "<YOUR_DB_PASSWORD>" or
refer to an env var like "${POSTGRES_PASSWORD}") and update the example comment
text to instruct users to set a secure password before deploying.
In `@packages/shared/alembic/versions/0001_add_user_settings_table.py`:
- Around line 26-27: The migration defines created_at and updated_at as
non-nullable but omits a database-side default; update the Column definitions
for created_at and updated_at (in the migration that creates the user_settings
table) to include a server_default so raw SQL inserts won't fail — e.g. add
server_default=sa.text('now()') (or server_default=sa.text('CURRENT_TIMESTAMP'))
to both sa.Column("created_at", ...) and sa.Column("updated_at", ...), and keep
updated_at nullable=False so the DB fills timestamps even when the ORM default
is not applied.
In `@template.env`:
- Line 144: The SETTINGS_DATABASE_URL currently uses a relative SQLite path
(sqlite+aiosqlite:///data/fastfetchbot.db) which makes the DB location dependent
on the process CWD; update this default in the template to an absolute-path form
so the DB is always created at the intended location (use the absolute SQLite
URL form with four slashes) or alternatively add a clear comment in the template
next to SETTINGS_DATABASE_URL documenting the required working directory; modify
the template entry labeled SETTINGS_DATABASE_URL accordingly to use the absolute
path or add the documentation note.
---
Nitpick comments:
In `@apps/telegram-bot/core/handlers/messages.py`:
- Line 24: The Ruff ARG001 false positive is triggered by the protocol-required
parameter context in the function all_messages_process; silence it by either
renaming the unused parameter to _context or appending a lint suppression
comment to the parameter like "context: CallbackContext # noqa: ARG001" so the
handler signature required by python-telegram-bot is preserved while suppressing
Ruff. Update the function definition in all_messages_process accordingly and
ensure no other references to context remain or are renamed if you choose the
underscore approach.
In `@apps/telegram-bot/pyproject.toml`:
- Line 6: The dependency line currently unconditionally adds the postgres extra
("fastfetchbot-shared[postgres]") which forces asyncpg to be installed; change
it to the plain package name ("fastfetchbot-shared") and add an opt-in extras
entry for this project (e.g., define a telegram-bot-postgres optional
dependency/extras group in this pyproject.toml) so deployers can opt into
Postgres support, or document using environment-specific extras to install
"fastfetchbot-shared[postgres]" when needed; update the dependency reference in
apps/telegram-bot/pyproject.toml (the line containing
"fastfetchbot-shared[postgres]") and add the corresponding optional extras
section for the telegram bot.
In `@packages/shared/alembic/env.py`:
- Around line 20-24: The default DB URL string is duplicated; replace the local
_DEFAULT_DATABASE_URL in env.py by importing the single source of truth (e.g.,
the constant defined in engine.py or a new shared constant module) and update
get_url() to return os.environ.get("SETTINGS_DATABASE_URL",
<imported_constant>); remove the duplicate literal so changes only need to be
made in one place and ensure the imported symbol name matches the one exported
by engine.py (or create/export a shared DEFAULT_DATABASE_URL constant).
In `@packages/shared/alembic/versions/0001_add_user_settings_table.py`:
- Around line 23-25: The server_default for the sa.Column "auto_fetch_in_dm"
should use SQLAlchemy's boolean expression instead of the raw string; update the
Column definition in the migration (the sa.Column for "auto_fetch_in_dm") to use
sa.sql.expression.true() (or sa.true()) as the server_default so
Alembic/autogenerate will render DEFAULT true/1 consistently across backends.
In `@packages/shared/fastfetchbot_shared/database/models/__init__.py`:
- Line 1: The line re-exporting UserSetting uses an unnecessary "# noqa: F401"
comment because F401 is not enabled in Ruff; remove the unused directive from
the import (the import statement "from
fastfetchbot_shared.database.models.user_setting import UserSetting" in
__init__.py) so the file no longer contains the inert "# noqa: F401" comment, or
alternatively enable F401 in your project Ruff config if you intend to keep
suppressing unused-import warnings.
In `@packages/shared/pyproject.toml`:
- Around line 16-17: Move the heavy DB deps out of the base dependencies so
consumers aren't forced to install them: remove "sqlalchemy[asyncio]>=2.0.0" and
"aiosqlite>=0.17.0" from the main dependencies in fastfetchbot-shared's
pyproject.toml and add them to a new optional extras group (e.g.
[project.optional-dependencies] database =
["sqlalchemy[asyncio]>=2.0.0","aiosqlite>=0.17.0"] or use a name like "sqlite");
then update the consumer that needs the DB (telegram-bot) to declare its
dependency on fastfetchbot-shared[database] (or [sqlite]) so only that package
pulls in these libs.
In `@packages/shared/tests/test_user_setting.py`:
- Around line 74-115: The test currently re-implements the "ensure" behavior
inline instead of exercising the actual service; update this test to call the
real ensure_user_settings function from core.services.user_settings (or add a
new integration test) rather than manually adding UserSetting rows: locate the
test function test_ensure_user_settings_creates_row and replace the manual
creation/commit logic that uses UserSetting(telegram_user_id=...) and direct
db_session queries with calls to ensure_user_settings(db_session,
telegram_user_id=user_id) (or call it twice to test create-then-no-op), then
assert on the returned or fetched UserSetting.auto_fetch_in_dm and created_at to
verify behavior and ensure regressions in ensure_user_settings are caught.
Comment on lines
+11
to
+21
| async def ensure_user_settings(user_id: int) -> None: | ||
| """Create a UserSetting row with defaults if one doesn't exist yet.""" | ||
| if user_id in _known_user_ids: | ||
| return | ||
| async with get_session() as session: | ||
| result = await session.execute( | ||
| select(UserSetting).where(UserSetting.telegram_user_id == user_id) | ||
| ) | ||
| if result.scalar_one_or_none() is None: | ||
| session.add(UserSetting(telegram_user_id=user_id)) | ||
| _known_user_ids.add(user_id) |
Contributor
There was a problem hiding this comment.
Possible IntegrityError under concurrent requests for the same new user.
If two messages from the same (new) user arrive near-simultaneously, both coroutines can pass the cache check, both query and find no row, and both attempt to insert. The second commit will raise an IntegrityError on the primary key. While narrow, this is possible in asyncio due to await suspension points.
A simple fix is to catch IntegrityError and treat it as a no-op:
🛡️ Proposed fix
+from sqlalchemy.exc import IntegrityError
+
async def ensure_user_settings(user_id: int) -> None:
"""Create a UserSetting row with defaults if one doesn't exist yet."""
if user_id in _known_user_ids:
return
- async with get_session() as session:
- result = await session.execute(
- select(UserSetting).where(UserSetting.telegram_user_id == user_id)
- )
- if result.scalar_one_or_none() is None:
- session.add(UserSetting(telegram_user_id=user_id))
+ try:
+ async with get_session() as session:
+ result = await session.execute(
+ select(UserSetting).where(UserSetting.telegram_user_id == user_id)
+ )
+ if result.scalar_one_or_none() is None:
+ session.add(UserSetting(telegram_user_id=user_id))
+ except IntegrityError:
+ pass # Another coroutine already created the row
_known_user_ids.add(user_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/telegram-bot/core/services/user_settings.py` around lines 11 - 21,
ensure_user_settings can raise IntegrityError if two coroutines concurrently
insert the same new user: keep the existing cache check and DB query in
ensure_user_settings, but wrap the session add/commit of
UserSetting(telegram_user_id=user_id) in a try/except that catches
sqlalchemy.exc.IntegrityError, performs session.rollback() (or ensures the
transaction is aborted) and treats the error as a no-op; still add user_id to
_known_user_ids after the DB operation to avoid future races. Reference
ensure_user_settings, _known_user_ids, get_session, and UserSetting when making
the change.
Comment on lines
+26
to
+27
| sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), | ||
| sa.Column("updated_at", sa.DateTime(timezone=True), nullable=False), |
Contributor
There was a problem hiding this comment.
created_at / updated_at lack a server_default — direct SQL inserts will violate NOT NULL
The ORM model handles these timestamps via Python-side default=, but the migration defines them as NOT NULL with no server_default. Any INSERT that bypasses the ORM (raw SQL, admin tooling, data-migration scripts) will fail.
Values specified with server_default are handled by the database and will automatically fill in defaults, even for raw SQL — avoiding potential exceptions.
🛠️ Proposed fix
- sa.Column("created_at", sa.DateTime(timezone=True), nullable=False),
- sa.Column("updated_at", sa.DateTime(timezone=True), nullable=False),
+ sa.Column(
+ "created_at",
+ sa.DateTime(timezone=True),
+ server_default=sa.func.now(),
+ nullable=False,
+ ),
+ sa.Column(
+ "updated_at",
+ sa.DateTime(timezone=True),
+ server_default=sa.func.now(),
+ nullable=False,
+ ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/alembic/versions/0001_add_user_settings_table.py` around
lines 26 - 27, The migration defines created_at and updated_at as non-nullable
but omits a database-side default; update the Column definitions for created_at
and updated_at (in the migration that creates the user_settings table) to
include a server_default so raw SQL inserts won't fail — e.g. add
server_default=sa.text('now()') (or server_default=sa.text('CURRENT_TIMESTAMP'))
to both sa.Column("created_at", ...) and sa.Column("updated_at", ...), and keep
updated_at nullable=False so the DB fills timestamps even when the ORM default
is not applied.
| # SQLAlchemy async database URL for user settings. | ||
| # SQLite (default): sqlite+aiosqlite:///data/fastfetchbot.db | ||
| # PostgreSQL: postgresql+asyncpg://user:password@host:5432/dbname | ||
| SETTINGS_DATABASE_URL=sqlite+aiosqlite:///data/fastfetchbot.db |
Contributor
There was a problem hiding this comment.
Relative SQLite path depends on process CWD
sqlite+aiosqlite:///data/fastfetchbot.db uses three slashes, making it CWD-relative. If the bot is launched from a directory other than the expected root (e.g., the uvicorn process starts from /), the DB file is created in the wrong location and any mounted volume won't capture it. Consider either:
- Documenting the expected working directory clearly, or
- Using an absolute path (four slashes:
sqlite+aiosqlite:////data/fastfetchbot.db) as the default so the location is unambiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@template.env` at line 144, The SETTINGS_DATABASE_URL currently uses a
relative SQLite path (sqlite+aiosqlite:///data/fastfetchbot.db) which makes the
DB location dependent on the process CWD; update this default in the template to
an absolute-path form so the DB is always created at the intended location (use
the absolute SQLite URL form with four slashes) or alternatively add a clear
comment in the template next to SETTINGS_DATABASE_URL documenting the required
working directory; modify the template entry labeled SETTINGS_DATABASE_URL
accordingly to use the absolute path or add the documentation note.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… into user-settings
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… into user-settings
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/telegram-bot/core/handlers/messages.py (2)
30-35: Swallowed exception loses the root cause in the warning log.
except Exception:without binding the exception means thelogger.warningcall emits no error message, type, or traceback — making it impossible to diagnose DB failures in production.🛠️ Proposed fix
- except Exception: + except Exception as e: logger.warning( - "Failed to ensure user settings for user {}", message.from_user.id + "Failed to ensure user settings for user {}: {}", message.from_user.id, e )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/telegram-bot/core/handlers/messages.py` around lines 30 - 35, The except block around await ensure_user_settings(message.from_user.id) is swallowing the exception; change it to capture the exception (e.g., except Exception as e) and include the exception details in the log (use logger.exception or logger.warning with exc_info=True and include e) so the warning for user id message.from_user.id contains the error type/message and stacktrace for debugging.
24-24: Rename unusedcontextto_contextto signal intentional non-use.Ruff (ARG001) flags
contextas unused. Since python-telegram-bot requires this positional parameter in all handler signatures, the idiomatic fix is to prefix with_.♻️ Proposed fix
-async def all_messages_process(update: Update, context: CallbackContext) -> None: +async def all_messages_process(update: Update, _context: CallbackContext) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/telegram-bot/core/handlers/messages.py` at line 24, The handler all_messages_process has an unused positional parameter named context which triggers the ARG001 lint; rename the parameter to _context in the function signature (async def all_messages_process(update: Update, _context: CallbackContext) -> None) to signal intentional non-use while preserving the required positional parameter for python-telegram-bot and avoid changes to the function body or call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/telegram-bot/core/handlers/messages.py`:
- Around line 30-35: The except block around await
ensure_user_settings(message.from_user.id) is swallowing the exception; change
it to capture the exception (e.g., except Exception as e) and include the
exception details in the log (use logger.exception or logger.warning with
exc_info=True and include e) so the warning for user id message.from_user.id
contains the error type/message and stacktrace for debugging.
- Line 24: The handler all_messages_process has an unused positional parameter
named context which triggers the ARG001 lint; rename the parameter to _context
in the function signature (async def all_messages_process(update: Update,
_context: CallbackContext) -> None) to signal intentional non-use while
preserving the required positional parameter for python-telegram-bot and avoid
changes to the function body or call sites.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Infrastructure / Ops
Tests