bittabola/tarjimon
1
0
Fork 0
Code Issues Pull requests Packages 1 Activity Actions

Auto: refactor/code-review-fixes #28

Merged
claude-bot merged 9 commits from refactor/code-review-fixes into main 2026-03-02 22:23:11 -07:00
Conversation 1 Commits 9 Files changed 11 +223 -269
claude-bot commented 2026-03-02 22:17:12 -07:00
Collaborator
Copy link

Automated PR for branch refactor/code-review-fixes.

Automated PR for branch `refactor/code-review-fixes`.
The timestamp was appended before the check, making the effective
limit N-1. Now we count first and only record allowed requests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Uses set.remove() in a try/except to atomically check-and-clear,
preventing double-processing on rapid messages. Moves httpx import
to module level to avoid per-call import overhead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replaces 20+ DatabaseManager() instantiations with a single _db
module-level singleton. Adds read_only parameter to get_connection()
to skip unnecessary commits on SELECT-only operations.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prevents concurrent payment webhooks from double-extending
subscriptions via a read-modify-write race.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extracts _build_translation_content() used by both streaming and
  non-streaming translation functions
- Defines _UZBEK_ALREADY_MARKERS and _image_label_for() helper to
  replace 4 duplicate sentinel string checks
- Removes unreachable post-loop code and dead local variable

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The premium status was already fetched in translate_message but
get_stats_button was fetching it again. Now accepts an optional
is_premium parameter.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The pattern of fetching subscription, extracting expires_at,
formatting date, and computing days_remaining was duplicated in
4 functions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replaces the character-by-character shrink loop with a single
byte slice + decode, handling multibyte boundaries via errors='ignore'.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
refactor: defer bot initialization to lifespan, move imports to top
All checks were successful
Auto PR Review / review (push) Successful in 6m40s
Details
2652d57ebf 
Bot application is now created inside lifespan() after config
validation, preventing confusing import-time crashes when
TELEGRAM_BOT_TOKEN is unset. Handler registration extracted
into _register_handlers(). Removes all # noqa: E402 comments.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
claude-bot commented 2026-03-02 22:23:10 -07:00
Author
Collaborator
Copy link

Approved

A well-structured refactoring that introduces a database singleton (_db), adds read_only connection optimization, defers bot initialization to ASGI lifespan, extracts helper functions to eliminate duplication, and fixes real bugs (off-by-one rate limiting, O(n²) truncation, TOCTOU in feedback state). All retry loop paths were verified to be exhaustive (aided by a catch-all except Exception in both functions), and the BEGIN EXCLUSIVE in activate_premium correctly prevents race conditions from duplicate payment webhooks. No critical or warning-level issues found.

Suggestions

  • handlers/translation.py: After removing the 'should not reach here' fallback from _perform_single_model_translation and _perform_streaming_translation, both functions lack a return statement after the for loop. This is safe at runtime (every loop iteration returns, and MAX_ATTEMPTS=3), but static analyzers like mypy cannot prove the loop body always returns and may flag a missing return type. Consider adding a defensive return S.GENERIC_ERROR, 0, 0, 0 or an assert False, 'unreachable' after each loop for type-checker clarity.
  • database.py: The singleton is named _db (underscore-prefixed, conventionally private) yet it is imported across multiple modules (admin_dashboard.py, webhook.py). This works fine but is a slight API signal mismatch. If it's intended as the public entry point for raw DB access, consider naming it db; if raw access should remain discouraged, the current naming is appropriate — just noting the inconsistency.
  • database.py: The read_only parameter on get_connection() controls whether commit() is called but doesn't prevent DML execution. This is reasonable (enforcing true read-only would be over-engineering), but a brief note in the docstring like 'Caller is responsible for not issuing DML when read_only=True' would clarify intent for future contributors.
  • database.py: In activate_premium, the explicit conn.execute('BEGIN EXCLUSIVE') is a good addition for preventing race conditions. Minor note: with Python's sqlite3 legacy transaction control (isolation_level=""), this works correctly because PRAGMAs don't trigger implicit BEGIN. If the project ever switches to autocommit=True (Python 3.12+), this code would need no changes since explicit BEGIN is the expected pattern in that mode.
## Approved A well-structured refactoring that introduces a database singleton (`_db`), adds `read_only` connection optimization, defers bot initialization to ASGI lifespan, extracts helper functions to eliminate duplication, and fixes real bugs (off-by-one rate limiting, O(n²) truncation, TOCTOU in feedback state). All retry loop paths were verified to be exhaustive (aided by a catch-all `except Exception` in both functions), and the `BEGIN EXCLUSIVE` in `activate_premium` correctly prevents race conditions from duplicate payment webhooks. No critical or warning-level issues found. ### Suggestions - **handlers/translation.py**: After removing the 'should not reach here' fallback from `_perform_single_model_translation` and `_perform_streaming_translation`, both functions lack a return statement after the `for` loop. This is safe at runtime (every loop iteration returns, and MAX_ATTEMPTS=3), but static analyzers like mypy cannot prove the loop body always returns and may flag a missing return type. Consider adding a defensive `return S.GENERIC_ERROR, 0, 0, 0` or an `assert False, 'unreachable'` after each loop for type-checker clarity. - **database.py**: The singleton is named `_db` (underscore-prefixed, conventionally private) yet it is imported across multiple modules (`admin_dashboard.py`, `webhook.py`). This works fine but is a slight API signal mismatch. If it's intended as the public entry point for raw DB access, consider naming it `db`; if raw access should remain discouraged, the current naming is appropriate — just noting the inconsistency. - **database.py**: The `read_only` parameter on `get_connection()` controls whether `commit()` is called but doesn't prevent DML execution. This is reasonable (enforcing true read-only would be over-engineering), but a brief note in the docstring like 'Caller is responsible for not issuing DML when read_only=True' would clarify intent for future contributors. - **database.py**: In `activate_premium`, the explicit `conn.execute('BEGIN EXCLUSIVE')` is a good addition for preventing race conditions. Minor note: with Python's sqlite3 legacy transaction control (`isolation_level=""`), this works correctly because PRAGMAs don't trigger implicit `BEGIN`. If the project ever switches to `autocommit=True` (Python 3.12+), this code would need no changes since explicit `BEGIN` is the expected pattern in that mode.
claude-bot deleted branch refactor/code-review-fixes 2026-03-02 22:23:11 -07:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
bittabola/tarjimon!28
No description provided.