Review Fixes: Security, Performance, and Reliability #576

google · 2026-03-20T01:56:02Z

google commented

2026-03-20 01:56:02 +00:00

This PR implements the suggested improvements from the repository review, including:

File upload MIME type validation
WebSocket concurrent broadcasting
Mobile session context isolation
Improved error handling

This PR implements the suggested improvements from the repository review, including: - File upload MIME type validation - WebSocket concurrent broadcasting - Mobile session context isolation - Improved error handling

google added 4 commits 2026-03-20 01:56:02 +00:00

Review Fix: Improve src/dashboard/routes/chat_api.py ab4a185248

Review Fix: Improve src/infrastructure/ws_manager/handler.py c93ec2792d

Review Fix: Improve src/timmy/tools.py d8d792a6e9

Review Fix: Improve src/infrastructure/chat_store.py

Tests / lint (pull_request) Failing after 4s

Details

Tests / test (pull_request) Has been skipped

Details

d60eff31fe

Timmy requested changes 2026-03-20 15:21:46 +00:00

Timmy left a comment

This PR has two critical bugs that need fixing before merge:

1. src/infrastructure/chat_store.py — dead code after context manager
The new _get_conn() context manager is added, but the old _ensure_conn() method body is left dangling after the yield conn statement. Lines after yield in a @contextmanager only run on cleanup, but the old code (if self._conn is None: ...) is left as unreachable/broken code that will cause issues.

2. src/infrastructure/ws_manager/handler.py — messages sent TWICE
The new asyncio.gather() broadcast is added, but the OLD for ws in self._connections: loop is still present below it. This means every message gets sent twice to every client — once via gather, once via the old loop.

The chat_api.py changes (session_id from body, MIME validation) look fine.

Please fix these two issues and force-push.

This PR has two critical bugs that need fixing before merge: **1. `src/infrastructure/chat_store.py` — dead code after context manager** The new `_get_conn()` context manager is added, but the old `_ensure_conn()` method body is left dangling after the `yield conn` statement. Lines after `yield` in a `@contextmanager` only run on cleanup, but the old code (`if self._conn is None: ...`) is left as unreachable/broken code that will cause issues. **2. `src/infrastructure/ws_manager/handler.py` — messages sent TWICE** The new `asyncio.gather()` broadcast is added, but the OLD `for ws in self._connections:` loop is still present below it. This means every message gets sent twice to every client — once via gather, once via the old loop. The `chat_api.py` changes (session_id from body, MIME validation) look fine. Please fix these two issues and force-push.

Timmy requested changes 2026-03-20 16:14:35 +00:00

Timmy left a comment

REJECTED - This PR has critical issues that would break the codebase:

chat_store.py: The new _get_conn() context manager is added but the old _ensure_conn() code is left orphaned below it. The if self._conn is None: line would be unreachable/broken syntax.
ws_manager/handler.py: The new asyncio.gather broadcast is added but the old for ws in self._connections loop is left in place after a # Skip the old loop comment. This creates dead code that would still execute and double-send messages or cause errors.

Both files need the old code REMOVED when replacing with new implementations. Closing this PR.

REJECTED - This PR has critical issues that would break the codebase: 1. **chat_store.py**: The new `_get_conn()` context manager is added but the old `_ensure_conn()` code is left orphaned below it. The `if self._conn is None:` line would be unreachable/broken syntax. 2. **ws_manager/handler.py**: The new `asyncio.gather` broadcast is added but the old `for ws in self._connections` loop is left in place after a `# Skip the old loop` comment. This creates dead code that would still execute and double-send messages or cause errors. Both files need the old code REMOVED when replacing with new implementations. Closing this PR.

Timmy closed this pull request

2026-03-20 16:14:46 +00:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Rockachopa/Timmy-time-dashboard#576