Conversation
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds a notification subsystem: Redis-backed caching, Supabase queueing, Expo push delivery, APScheduler-based processing, webhook enqueueing, PostHog hooks, configuration entries, CI workflow, and extensive unit/integration tests for the new services. Changes
Sequence DiagramssequenceDiagram
actor Webhook
participant EnqueueService as EnqueueSvc
participant CacheService as Cache
participant Supabase
participant Redis
participant NotificationService as NotifySvc
note over Webhook,NotifySvc `#DDEBF7`: Entry creation triggers notification enqueueing
Webhook->>EnqueueSvc: enqueue_entry_notification(entry)
EnqueueSvc->>Cache: get_notification_settings_batch(user_ids)
alt cached
Cache-->>EnqueueSvc: settings (cached)
else not cached
Cache->>Redis: mget(keys)
Redis-->>Cache: misses
Cache->>Supabase: query(notification_settings)
Supabase-->>Cache: settings
Cache->>Redis: setex(cached_settings)
Cache-->>EnqueueSvc: settings
end
EnqueueSvc->>Cache: get_push_tokens_batch(filtered_user_ids)
alt cached
Cache-->>EnqueueSvc: tokens (cached)
else not cached
Cache->>Redis: mget(token_keys)
Redis-->>Cache: misses
Cache->>Supabase: query(push_tokens)
Supabase-->>Cache: tokens
Cache->>Redis: setex(cached_tokens)
Cache-->>EnqueueSvc: tokens
end
EnqueueSvc->>NotifySvc: enqueue_notification(payload)
NotifySvc->>Supabase: rpc.enqueue(...) (queue insert)
Supabase-->>NotifySvc: success
NotifySvc-->>EnqueueSvc: enqueue result
EnqueueSvc-->>Webhook: 200 OK
sequenceDiagram
participant Scheduler
participant NotificationService as NotifySvc
participant Supabase
participant Expo
participant DLQ as Supabase_DLQ
note over Scheduler,NotifySvc `#F7F3E8`: Periodic queue processing job
Scheduler->>NotifySvc: process_queue()
NotifySvc->>Supabase: rpc.read_messages(batch_size, visibility_timeout)
Supabase-->>NotifySvc: messages[]
loop for each message
NotifySvc->>Expo: send push messages (concurrent, semaphore)
alt success
Expo-->>NotifySvc: OK
NotifySvc->>Supabase: rpc.delete_message(msg_id)
else transient failure / rate-limit
Expo-->>NotifySvc: error (rate-limit)
NotifySvc->>NotifySvc: backoff & retry
alt max retries exceeded or persistent failure
NotifySvc->>DLQ: rpc.move_to_dlq(msg)
end
end
end
NotifySvc-->>Scheduler: stats (processed/succeeded/failed/moved_to_dlq)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
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 |
|
Note Docstrings generation - SUCCESS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (17)
backend/tests/test_cache_service.py (2)
7-11: Consider using pytest configuration instead of manual sys.path manipulation.Manual
sys.pathmanipulation can be fragile and may cause issues in different environments. Consider configuring the Python path viapytest.iniorpyproject.tomlinstead.🔎 Alternative approach
Create a
pytest.iniin the backend directory:[pytest] pythonpath = . testpaths = testsThen remove the sys.path manipulation from the test file:
-import os import sys import pytest import json from unittest.mock import MagicMock, patch, Mock - -# Ensure the backend directory (which contains `services/`) is on sys.path -CURRENT_DIR = os.path.dirname(__file__) -BACKEND_DIR = os.path.abspath(os.path.join(CURRENT_DIR, os.pardir)) -if BACKEND_DIR not in sys.path: - sys.path.insert(0, BACKEND_DIR) -
173-173: Remove unused parameter.The
mock_supabase_clientparameter is not used in this test function. While not harmful, removing it makes the test signature cleaner.🔎 Proposed fix
-def test_get_notification_settings_no_redis(cache_service_no_redis, mock_supabase_client): +def test_get_notification_settings_no_redis(cache_service_no_redis): """Test getting notification settings when Redis is not available."""Based on static analysis hints.
backend/tests/test_notification_enqueue_service_integration.py (5)
7-11: Consider using proper package structure instead of sys.path manipulation.While this sys.path approach works, consider either:
- Installing the package in editable mode (
pip install -e .) with a proper setup.py/pyproject.toml- Using pytest's conftest.py at the project root to handle imports
This makes tests more maintainable and portable across different environments.
32-74: Fixture complexity with manual attribute assignment.The fixture manually assigns internal attributes (lines 70-73) after instantiation, which can be fragile if the service's initialization logic changes. Consider whether the service could accept dependencies via constructor injection instead, which would make testing cleaner.
However, for integration tests with multiple external dependencies, this approach is acceptable.
97-109: String-based discrimination in side_effect is fragile.The side_effect function distinguishes between notification_settings and push_tokens calls by checking if "notification_settings" is in the key string. This approach is fragile and will break if key naming conventions change.
Consider using separate mock objects for different call contexts or tracking call order more explicitly.
174-175: Weak assertion on cache operations.The assertion
assert mock_redis_client.setex.call_count >= 2doesn't verify exactly what was cached or in what order. Consider asserting the exact count and potentially verifying the keys/values that were set.
275-299: Testing private method directly.While this test is clear and useful, testing private methods (_filter_recipients_by_notification_settings) directly can make refactoring harder. Consider whether this behavior is sufficiently covered by the public API tests (e.g., test_enqueue_entry_notification_with_cache_hit).
However, for complex helper methods, direct testing is acceptable and often beneficial.
backend/tests/test_notification_service.py (3)
94-118: Consider asserting that no RPC call is made.The test verifies that
result is Falsefor missing fields, but doesn't confirm that the validation fails early without making any RPC calls. Consider adding assertions like:notification_service.supabase.rpc.assert_not_called()after each validation test to ensure efficient fail-fast behavior.
246-246: Weak assertion on RPC call count.The assertion
assert mock_supabase_client.rpc.call_count >= 2doesn't verify the specific operations (delete + send to DLQ). Consider asserting the exact call count and potentially usingassert_has_callsto verify the sequence of operations.
327-333: Remove unused kwargs parameter.The
mock_rpc_side_effectfunction declares**kwargsbut never uses it. Remove it from the signature:-def mock_rpc_side_effect(*args, **kwargs): +def mock_rpc_side_effect(*args):Based on static analysis hint.
backend/services/notification_scheduler.py (1)
48-55: Consider tracking consecutive failures for alerting.The scheduler will continue running even if
process_queue()repeatedly fails, which is good for resilience. However, you might want to track consecutive failures and trigger alerts (e.g., via PostHog or metrics) if the queue processor fails multiple times in a row, as this could indicate a systemic issue.This is optional and depends on your monitoring strategy.
backend/services/redis_client.py (1)
39-45: Connection pool lifecycle and cleanup.The ConnectionPool created here lives for the application lifetime, which is appropriate for a singleton client. However, there's no explicit cleanup/shutdown mechanism. Consider whether the application's shutdown lifecycle properly closes Redis connections, or if a shutdown hook is needed.
For most applications, this isn't critical as the OS will clean up connections on process exit, but explicit cleanup is better practice for testing and graceful shutdowns.
backend/services/cache_service.py (3)
58-60: Consider usinglogging.exceptionfor better error context.When logging errors in exception handlers,
logging.exceptionautomatically includes the stack trace, which is more helpful for debugging thanlogging.errorwithexc_info=True.🔎 Proposed refactor
- except Exception as e: - logger.error(f"Error fetching notification settings from Supabase for user {user_id}: {str(e)}") + except Exception as e: + logger.exception(f"Error fetching notification settings from Supabase for user {user_id}: {e}")
119-120: Consider usinglogging.exceptionfor better error context.Similar to the single-fetch path, using
logging.exceptionhere would provide more debugging context.🔎 Proposed refactor
- except Exception as e: - logger.error(f"Error batch fetching notification settings from Supabase: {str(e)}") + except Exception as e: + logger.exception(f"Error batch fetching notification settings from Supabase: {e}")
226-227: Consider usinglogging.exceptionfor better error context.Using
logging.exceptionwould provide more debugging context here as well.🔎 Proposed refactor
- except Exception as e: - logger.error(f"Error batch fetching push tokens from Supabase: {str(e)}") + except Exception as e: + logger.exception(f"Error batch fetching push tokens from Supabase: {e}")backend/tests/test_notification_service_integration.py (1)
431-438: Remove or replace unused result variable.The
resultvariable is assigned but never used. Consider replacing it with_or removing the assignment.🔎 Proposed refactor
- result = await notification_service._send_notification( + _ = await notification_service._send_notification( title="Test", body="Test", recipients=["token"], priority="default", retry_count=0, max_retries=3 )backend/services/notification_enqueue_service.py (1)
177-179: Consider usinglogging.exceptionfor better error context.Using
logging.exceptionwould automatically include the stack trace for debugging.🔎 Proposed refactor
- except Exception as e: - logger.error(f"Error fetching user profile {user_id}: {str(e)}") + except Exception as e: + logger.exception(f"Error fetching user profile {user_id}: {e}")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/test.ymlbackend/.gitignorebackend/config.pybackend/main.pybackend/requirements.txtbackend/routers/webhooks.pybackend/services/cache_service.pybackend/services/notification_enqueue_service.pybackend/services/notification_scheduler.pybackend/services/notification_service.pybackend/services/redis_client.pybackend/tests/test_cache_service.pybackend/tests/test_notification_enqueue_service_integration.pybackend/tests/test_notification_service.pybackend/tests/test_notification_service_integration.py
🧰 Additional context used
🧬 Code graph analysis (7)
backend/services/cache_service.py (2)
backend/services/redis_client.py (1)
get_redis_client(18-58)backend/services/supabase_client.py (1)
get_supabase_client(4-9)
backend/tests/test_notification_enqueue_service_integration.py (1)
backend/services/notification_enqueue_service.py (3)
enqueue_entry_notification(60-170)_filter_recipients_by_notification_settings(199-239)_get_push_tokens_for_users(241-269)
backend/routers/webhooks.py (2)
backend/services/notification_enqueue_service.py (2)
NotificationEnqueueService(50-269)enqueue_entry_notification(60-170)backend/services/ingestion_service.py (2)
ingest_entry(17-115)IngestionService(11-145)
backend/services/redis_client.py (1)
backend/services/pinecone_client.py (1)
get_pinecone_client(7-17)
backend/tests/test_notification_service_integration.py (2)
backend/services/notification_service.py (5)
NotificationService(18-479)enqueue_notification(51-121)process_queue(123-177)process_dlq(461-479)_send_notification(251-327)backend/services/notification_scheduler.py (4)
NotificationScheduler(10-55)start(19-36)stop(38-46)_process_queue_job(48-55)
backend/services/notification_enqueue_service.py (4)
backend/services/notification_service.py (2)
NotificationService(18-479)enqueue_notification(51-121)backend/services/supabase_client.py (1)
get_supabase_client(4-9)backend/services/cache_service.py (2)
get_notification_settings_batch(62-122)get_push_tokens_batch(163-229)backend/models.py (2)
Entry(45-59)EntryShare(71-75)
backend/main.py (1)
backend/services/notification_scheduler.py (3)
NotificationScheduler(10-55)start(19-36)stop(38-46)
🪛 actionlint (1.7.9)
.github/workflows/test.yml
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
21-21: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 Ruff (0.14.10)
backend/tests/test_notification_service.py
327-327: Unused function argument: kwargs
(ARG001)
backend/services/notification_scheduler.py
55-55: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/tests/test_cache_service.py
173-173: Unused function argument: mock_supabase_client
(ARG001)
backend/services/cache_service.py
56-56: Consider moving this statement to an else block
(TRY300)
58-58: Do not catch blind exception: Exception
(BLE001)
59-59: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
59-59: Use explicit conversion flag
Replace with conversion flag
(RUF010)
92-92: Use explicit conversion flag
Replace with conversion flag
(RUF010)
96-96: Do not catch blind exception: Exception
(BLE001)
97-97: Use explicit conversion flag
Replace with conversion flag
(RUF010)
119-119: Do not catch blind exception: Exception
(BLE001)
120-120: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
120-120: Use explicit conversion flag
Replace with conversion flag
(RUF010)
157-157: Consider moving this statement to an else block
(TRY300)
159-159: Do not catch blind exception: Exception
(BLE001)
160-160: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
160-160: Use explicit conversion flag
Replace with conversion flag
(RUF010)
191-191: Use explicit conversion flag
Replace with conversion flag
(RUF010)
195-195: Do not catch blind exception: Exception
(BLE001)
196-196: Use explicit conversion flag
Replace with conversion flag
(RUF010)
226-226: Do not catch blind exception: Exception
(BLE001)
227-227: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
227-227: Use explicit conversion flag
Replace with conversion flag
(RUF010)
257-257: Consider moving this statement to an else block
(TRY300)
259-259: Do not catch blind exception: Exception
(BLE001)
260-260: Use explicit conversion flag
Replace with conversion flag
(RUF010)
286-286: Consider moving this statement to an else block
(TRY300)
288-288: Do not catch blind exception: Exception
(BLE001)
289-289: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/routers/webhooks.py
56-59: Abstract raise to an inner function
(TRY301)
66-66: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/services/redis_client.py
53-53: Consider moving this statement to an else block
(TRY300)
55-55: Do not catch blind exception: Exception
(BLE001)
56-56: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/services/notification_service.py
96-96: Local variable response is assigned to but never used
Remove assignment to unused variable response
(F841)
108-108: Consider moving this statement to an else block
(TRY300)
111-111: Use explicit conversion flag
Replace with conversion flag
(RUF010)
171-171: Use explicit conversion flag
Replace with conversion flag
(RUF010)
188-188: Local variable read_ct is assigned to but never used
Remove assignment to unused variable read_ct
(F841)
205-205: Local variable metadata is assigned to but never used
Remove assignment to unused variable metadata
(F841)
240-240: Use explicit conversion flag
Replace with conversion flag
(RUF010)
295-295: Consider moving this statement to an else block
(TRY300)
309-309: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
326-326: Use explicit conversion flag
Replace with conversion flag
(RUF010)
356-356: Local variable response is assigned to but never used
Remove assignment to unused variable response
(F841)
371-371: Use explicit conversion flag
Replace with conversion flag
(RUF010)
404-404: Use explicit conversion flag
Replace with conversion flag
(RUF010)
432-432: Use explicit conversion flag
Replace with conversion flag
(RUF010)
459-459: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/tests/test_notification_service_integration.py
97-97: Unused function argument: kwargs
(ARG001)
150-150: Unused function argument: kwargs
(ARG001)
214-214: Unused function argument: kwargs
(ARG001)
273-273: Unused function argument: kwargs
(ARG001)
322-322: Unused function argument: kwargs
(ARG001)
412-412: Unused function argument: args
(ARG001)
412-412: Unused function argument: kwargs
(ARG001)
431-431: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
479-479: Unused function argument: kwargs
(ARG001)
backend/services/notification_enqueue_service.py
166-166: Consider moving this statement to an else block
(TRY300)
169-169: Use explicit conversion flag
Replace with conversion flag
(RUF010)
176-176: Consider moving this statement to an else block
(TRY300)
177-177: Do not catch blind exception: Exception
(BLE001)
178-178: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
178-178: Use explicit conversion flag
Replace with conversion flag
(RUF010)
195-195: Do not catch blind exception: Exception
(BLE001)
196-196: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
196-196: Use explicit conversion flag
Replace with conversion flag
(RUF010)
234-234: Consider moving this statement to an else block
(TRY300)
236-236: Do not catch blind exception: Exception
(BLE001)
237-237: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
237-237: Use explicit conversion flag
Replace with conversion flag
(RUF010)
265-265: Consider moving this statement to an else block
(TRY300)
267-267: Do not catch blind exception: Exception
(BLE001)
268-268: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
268-268: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/main.py
66-66: Use explicit conversion flag
Replace with conversion flag
(RUF010)
76-76: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (37)
backend/.gitignore (1)
29-30: LGTM!The addition of the
blogs/directory to the ignore list is properly formatted and well-documented with a clear comment..github/workflows/test.yml (1)
1-51: Well-structured test workflow.The workflow is properly configured with:
- Appropriate triggers for PRs and pushes
- Path filtering to run only when backend changes
- Pip caching for faster builds
- Comprehensive environment variable configuration
- Proper secret management
backend/config.py (2)
8-16: Well-implemented helper function.The
_get_int_envhelper provides robust error handling for integer environment variables with proper fallback to defaults. Good defensive programming practice.
37-51: Comprehensive configuration additions.The new configuration sections for PostHog, Notification Service, and Redis are well-organized and appropriately typed. The use of
Optional[str]forREDIS_PASSWORDcorrectly handles the case where Redis may not require authentication.backend/tests/test_cache_service.py (1)
1-474: Excellent test coverage.This test suite demonstrates thorough testing practices:
- Cache hit/miss scenarios
- Batch operations with partial cache hits
- Error handling and resilience (Redis unavailable, connection errors)
- Edge cases (empty lists, None values)
- TTL verification
- JSON serialization/deserialization
The comprehensive coverage will help ensure the CacheService operates reliably in production.
backend/main.py (1)
24-25: Good error handling in lifecycle management.The try-except blocks around scheduler start/stop operations with proper logging ensure that errors during startup or shutdown are captured and logged appropriately. This defensive approach will help with debugging production issues.
Also applies to: 58-76
backend/tests/test_notification_enqueue_service_integration.py (2)
179-224: Good resilience test for Redis failures.This test properly validates that the system gracefully degrades when Redis is unavailable, ensuring the primary workflow (notification enqueueing) continues even when the cache layer fails. This is essential for production resilience.
303-323: Clear test for token aggregation logic.This test clearly validates the flattening of per-user token lists into a single list, which is an important behavior. The assertions verify both count and content.
backend/tests/test_notification_service.py (2)
56-90: Thorough test with detailed assertions.This test properly validates the entire enqueue flow, including JSON message serialization and parameter passing. The detailed assertions (lines 79-90) ensure correctness of the RPC call structure.
165-191: Good test for rate limit retry logic.This test properly validates the retry behavior on rate limit errors. The test confirms that transient failures are handled gracefully with automatic retries.
backend/routers/webhooks.py (1)
61-67: Good resilience pattern for notification enqueueing.The notification enqueueing is properly isolated in a try/except block that logs failures without breaking the primary webhook operation. This ensures that notification issues don't impact the core ingestion flow, which is the correct design for a secondary operation.
The
awaiton line 63 is correct for the async enqueue_entry_notification method.backend/services/cache_service.py (4)
14-23: LGTM!The initialization logic correctly handles both Redis-available and fallback scenarios with appropriate logging.
151-155: LGTM! Smart optimization for caching empty results.Caching empty token lists prevents repeated database queries for users without tokens, which is a thoughtful optimization.
231-261: LGTM!The implementation correctly handles JSON parsing with a fallback for plain string values, and gracefully degrades when Redis is unavailable.
263-290: LGTM!The implementation correctly serializes complex types and uses
setexfor atomic key-value-TTL operations.backend/tests/test_notification_service_integration.py (9)
52-114: LGTM!This test comprehensively covers the happy path from enqueue to successful processing with appropriate mocking and assertions.
117-178: LGTM!The test properly validates the DLQ flow, including verification that the failure count is incremented and the message is sent to the correct DLQ.
181-240: LGTM!This test correctly validates that messages exceeding the DLQ limit are discarded rather than moved to the DLQ.
243-289: LGTM!This test validates concurrent message processing with appropriate assertions on both stats and call counts.
292-342: LGTM!This test properly validates DLQ processing behavior.
345-351: LGTM!This test validates the scheduler's initial state correctly.
354-371: LGTM!This test properly validates the scheduler's start and stop lifecycle.
374-393: LGTM!This test validates that the scheduler job executes without errors.
446-494: LGTM!This test properly validates that batch size limits are enforced during queue processing.
backend/services/notification_enqueue_service.py (5)
10-47: LGTM!The TypedDict definitions provide good type safety for the service's data structures.
53-100: LGTM!The initialization is clean, and early validation of required fields with appropriate logging is well implemented.
136-156: LGTM!The notification construction includes appropriate metadata for tracking and a
page_urlin the data payload for navigation.
181-197: LGTM! Good bidirectional friendship query.The implementation correctly handles both directions of friendship relationships by querying where the user is either the requester or the friend.
236-239: Verify the fail-open error handling strategy.When filtering by notification settings fails, the code returns all
user_ids(fail-open). This means users could receive notifications they didn't opt into if there's a cache/database error. Consider whether failing closed (returning an empty list) would be more appropriate to respect user preferences.Do you want to keep the fail-open behavior, or should this fail closed to avoid sending unwanted notifications during outages?
backend/services/notification_service.py (8)
21-49: LGTM!The initialization properly handles optional PostHog configuration and sets up concurrency control with a semaphore.
51-121: LGTM!The enqueue logic properly validates inputs, constructs a well-structured message payload, and handles errors with PostHog logging.
123-177: LGTM! Efficient concurrent processing.The queue processing logic uses
asyncio.gatherfor concurrent message processing while respecting the semaphore-based concurrency limit, which is an efficient approach.
179-249: LGTM!The message processing logic correctly parses, validates, and handles both success and failure cases with appropriate stats tracking.
307-320: LGTM! Proper exponential backoff with rate limit handling.The exponential backoff implementation with jitter (using
random.uniform) and a 60-second cap is appropriate for handling rate limits. The use of non-cryptographic random is fine for this use case.
329-414: LGTM!The failure handling logic properly implements the DLQ pattern with appropriate limits and PostHog logging for discarded messages.
416-433: LGTM!The message deletion logic correctly propagates exceptions for proper error handling by callers.
461-479: LGTM! Clean DLQ processing with queue name swap.The DLQ processing implementation cleverly reuses
process_queueby temporarily swapping queue names, with proper cleanup in thefinallyblock.
| @app.on_event("startup") | ||
| async def startup_event(): | ||
| """Start background tasks on application startup.""" | ||
| logger.info("Starting up application...") | ||
| try: | ||
| notification_scheduler.start() | ||
| logger.info("Application startup complete") | ||
| except Exception as e: | ||
| logger.error(f"Error during startup: {str(e)}", exc_info=True) | ||
|
|
||
| @app.on_event("shutdown") | ||
| async def shutdown_event(): | ||
| """Stop background tasks on application shutdown.""" | ||
| logger.info("Shutting down application...") | ||
| try: | ||
| notification_scheduler.stop() | ||
| logger.info("Application shutdown complete") | ||
| except Exception as e: | ||
| logger.error(f"Error during shutdown: {str(e)}", exc_info=True) |
There was a problem hiding this comment.
Replace deprecated @app.on_event() with lifespan context manager.
The @app.on_event() decorator has been deprecated since FastAPI 0.109.0 and you're using FastAPI 0.115.0. The recommended approach is to use the lifespan context manager for startup and shutdown logic.
🔎 Proposed fix
+from contextlib import asynccontextmanager
+
+@asynccontextmanager
+async def lifespan(app: FastAPI):
+ """Application lifespan manager."""
+ # Startup
+ logger.info("Starting up application...")
+ try:
+ notification_scheduler.start()
+ logger.info("Application startup complete")
+ except Exception as e:
+ logger.error(f"Error during startup: {e!r}", exc_info=True)
+
+ yield
+
+ # Shutdown
+ logger.info("Shutting down application...")
+ try:
+ notification_scheduler.stop()
+ logger.info("Application shutdown complete")
+ except Exception as e:
+ logger.error(f"Error during shutdown: {e!r}", exc_info=True)
+
app = FastAPI(
title="KeepSafe Backend API",
description="Backend API for KeepSafe with vector search capabilities",
- version="1.0.0"
+ version="1.0.0",
+ lifespan=lifespan
)
# Initialize notification scheduler
notification_scheduler = NotificationScheduler()
-@app.on_event("startup")
-async def startup_event():
- """Start background tasks on application startup."""
- logger.info("Starting up application...")
- try:
- notification_scheduler.start()
- logger.info("Application startup complete")
- except Exception as e:
- logger.error(f"Error during startup: {str(e)}", exc_info=True)
-
-@app.on_event("shutdown")
-async def shutdown_event():
- """Stop background tasks on application shutdown."""
- logger.info("Shutting down application...")
- try:
- notification_scheduler.stop()
- logger.info("Application shutdown complete")
- except Exception as e:
- logger.error(f"Error during shutdown: {str(e)}", exc_info=True)
-Note: I've also replaced str(e) with {e!r} which is the recommended conversion flag for exception formatting in f-strings.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
66-66: Use explicit conversion flag
Replace with conversion flag
(RUF010)
76-76: Use explicit conversion flag
Replace with conversion flag
(RUF010)
| except Exception as e: | ||
| logger.warning(f"Failed to initialize Redis client: {str(e)}. Falling back to Supabase only.") | ||
| _redis_client = None | ||
| return None |
There was a problem hiding this comment.
Potential credential exposure in exception logging.
Line 56 logs the exception message with str(e), which might inadvertently expose sensitive information like passwords if the Redis connection error includes credentials in its message. Consider sanitizing the error message or using a more generic error description.
However, in practice, most Redis client errors don't expose credentials, so this may be a low risk.
🔎 Suggested approach
except Exception as e:
- logger.warning(f"Failed to initialize Redis client: {str(e)}. Falling back to Supabase only.")
+ logger.warning(f"Failed to initialize Redis client: {type(e).__name__}. Falling back to Supabase only.")Or use more detailed logging at a lower level that's not exposed to users:
except Exception as e:
+ logger.debug(f"Redis initialization error details: {str(e)}")
logger.warning(f"Failed to initialize Redis client. Falling back to Supabase only.")🧰 Tools
🪛 Ruff (0.14.10)
55-55: Do not catch blind exception: Exception
(BLE001)
56-56: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In backend/services/redis_client.py around lines 55–58, the warning currently
logs the raw exception string which could include sensitive credentials; change
the warning to a generic message like "Failed to initialize Redis client.
Falling back to Supabase only." and move the detailed exception information to a
debug-level log (or logger.debug/ logger.exception with exc_info=True) so full
error details are only recorded at a lower log level; keep the fallback behavior
(_redis_client = None; return None).
Docstrings generation was requested by @fortune710. * #11 (comment) The following files were modified: * `backend/config.py` * `backend/main.py` * `backend/routers/webhooks.py` * `backend/services/cache_service.py` * `backend/services/notification_enqueue_service.py` * `backend/services/notification_scheduler.py` * `backend/services/notification_service.py` * `backend/services/redis_client.py` * `backend/tests/test_cache_service.py` * `backend/tests/test_notification_enqueue_service_integration.py` * `backend/tests/test_notification_service.py` * `backend/tests/test_notification_service_integration.py`
📝 Add docstrings to `dev`
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
backend/services/notification_service.py (1)
463-473: Duplicate: PostHog shutdown() usage issue already flagged.An existing review comment correctly identifies that
shutdown()should not be called after each error capture. Useflush()instead.backend/main.py (1)
65-87: Replace deprecated@app.on_event()with lifespan context manager.The
@app.on_event()decorator has been deprecated since FastAPI 0.109.0 and you're using FastAPI 0.115.0. The recommended approach is to use thelifespancontext manager.Additionally, consider whether swallowing startup exceptions (lines 76-77) is intentional - if the scheduler fails to start, the application will continue running without background notification processing, which may not be the desired behavior.
backend/services/redis_client.py (1)
35-58: Consider sanitizing exception messages in logs.Line 57 logs
str(e)which could potentially expose sensitive connection details. Consider usingtype(e).__name__for the warning log and moving detailed error information to a debug-level log.backend/config.py (1)
55-57: Use the_get_int_envhelper for consistency.Lines 55-56 use
int()directly, which can raise exceptions for invalid values. The_get_int_envhelper was created to handle this safely. Line 57 has the same issue. This inconsistency could lead to runtime errors if invalid environment values are provided.🔎 Proposed fix
- NOTIFICATION_CONCURRENCY: int = int(os.getenv("NOTIFICATION_CONCURRENCY", "20")) - NOTIFICATION_BATCH_SIZE: int = int(os.getenv("NOTIFICATION_BATCH_SIZE", "100")) - NOTIFICATION_DLQ_LIMIT: int = int(os.getenv("NOTIFICATION_DLQ_LIMIT", "3")) + NOTIFICATION_CONCURRENCY: int = _get_int_env("NOTIFICATION_CONCURRENCY", 20) + NOTIFICATION_BATCH_SIZE: int = _get_int_env("NOTIFICATION_BATCH_SIZE", 100) + NOTIFICATION_DLQ_LIMIT: int = _get_int_env("NOTIFICATION_DLQ_LIMIT", 3)
🧹 Nitpick comments (7)
backend/services/notification_enqueue_service.py (1)
184-196: Consider usinglogging.exceptionfor consistency.The error handling is correct and fail-safe. For minor improvement, you could use
logger.exception()instead oflogger.error(..., exc_info=True)as it's more concise:logger.exception(f"Error fetching user profile {user_id}")This applies to other helper methods in this file as well.
backend/services/notification_service.py (2)
94-102: Consider using_for intentionally unused response.The
responsevariable is assigned but never used. If the response is not needed, assign to_to indicate intentional discard:_ = self.supabase.rpc( "pgmq_public.send", { "queue_name": self.queue_name, "msg": json.dumps(message) } ).execute()
194-220: Consider logging extracted metadata for observability.The
read_ctandmetadatavariables are extracted but not used. Consider logging them for better observability:logger.info(f"Processing message: msg_id={msg_id}, read_ct={read_ct}, metadata={metadata}")This would help track message retry patterns and custom tracking data.
backend/tests/test_notification_service_integration.py (1)
104-122: Consider extracting the duplicatedmock_rpc_side_effecthelper.The same
mock_rpc_side_effectfunction is defined identically in 6 different test functions. Consider extracting it to a shared fixture or module-level helper to reduce duplication.🔎 Proposed refactor
# Add as a fixture or module-level helper def create_mock_rpc_side_effect(read_response): """Factory to create mock RPC side effect with given read response.""" def mock_rpc_side_effect(*args, **kwargs): mock_rpc_result = MagicMock() if args[0] == "pgmq_public.read": mock_rpc_result.execute.return_value = read_response else: mock_rpc_result.execute.return_value = MagicMock() return mock_rpc_result return mock_rpc_side_effect # Usage in tests: mock_supabase_client.rpc.side_effect = create_mock_rpc_side_effect(read_response)Also applies to: 169-187, 245-263, 316-334, 377-395, 570-588
backend/tests/test_notification_service.py (1)
396-407: Test assertion could be strengthened.The
test_log_error_to_posthogtest only verifies that the method doesn't raise an exception. Consider adding an assertion to verify that PostHog'scapturemethod was called with the expected parameters.🔎 Proposed enhancement
def test_log_error_to_posthog(notification_service): """_log_error_to_posthog should capture error to PostHog.""" # Arrange error = Exception("Test error") context = {"operation": "test", "key": "value"} # Act notification_service._log_error_to_posthog(error, context) - # Assert - PostHog should have been called (mocked in fixture) - # The actual PostHog call is mocked, so we just verify it doesn't raise + # Assert - Verify PostHog capture was called + notification_service.posthog_client.capture.assert_called_once() + call_kwargs = notification_service.posthog_client.capture.call_args + assert call_kwargs[1]["event"] == "notification_error"backend/tests/test_notification_enqueue_service_integration.py (2)
179-202: Extract duplicatetable_side_effecthelper to reduce duplication.The
table_side_effectfunction is duplicated three times (lines 179-202, 255-278, and 321-344) with identical implementations. This creates maintenance burden and increases the risk of inconsistencies if changes are needed.🔎 Proposed refactor
Create a shared fixture or module-level helper function:
def create_table_side_effect(mock_profile_response, mock_settings_response, mock_tokens_response): """ Create a table side effect function for mocking Supabase table queries. Parameters: mock_profile_response: Mock response for profiles table mock_settings_response: Mock response for notification_settings table mock_tokens_response: Mock response for push_tokens table Returns: Callable: A function that returns appropriately configured MagicMock instances """ def table_side_effect(table_name): mock_table = MagicMock() if table_name == "profiles": mock_table.select.return_value.eq.return_value.single.return_value.execute.return_value = mock_profile_response elif table_name == "notification_settings": mock_table.select.return_value.in_.return_value.execute.return_value = mock_settings_response elif table_name == "push_tokens": mock_table.select.return_value.in_.return_value.execute.return_value = mock_tokens_response return mock_table return table_side_effectThen in each test, replace the duplicated function with:
mock_supabase_client.table.side_effect = create_table_side_effect( mock_profile_response, mock_settings_response, mock_tokens_response )Also applies to: 255-278, 321-344
117-138: Consider more robust cache key matching.The
mget_side_effectfunction uses string matching ("notification_settings" in str(keys[0])) to determine which cached data to return. This approach is fragile and tightly couples the test to the internal cache key naming convention. If the cache key format changes, this test will break silently or return incorrect data.💡 Suggestion
Consider using more explicit mock setup with call tracking or creating a mock cache dictionary:
# Create a mock cache dictionary mock_cache = { "notification_settings:user-1": json.dumps({"user_id": "user-1", "friend_activity": True}), "notification_settings:user-2": json.dumps({"user_id": "user-2", "friend_activity": True}), "push_tokens:user-1": json.dumps(["token1", "token2"]), "push_tokens:user-2": json.dumps(["token3"]) } def mget_side_effect(keys): return [mock_cache.get(key) for key in keys] mock_redis_client.mget.side_effect = mget_side_effectThis makes the test more explicit and resilient to internal implementation changes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/test.ymlbackend/.gitignorebackend/config.pybackend/main.pybackend/requirements.txtbackend/routers/webhooks.pybackend/services/cache_service.pybackend/services/notification_enqueue_service.pybackend/services/notification_scheduler.pybackend/services/notification_service.pybackend/services/redis_client.pybackend/tests/test_cache_service.pybackend/tests/test_notification_enqueue_service_integration.pybackend/tests/test_notification_service.pybackend/tests/test_notification_service_integration.py
🧰 Additional context used
🧬 Code graph analysis (9)
backend/routers/webhooks.py (2)
backend/services/notification_enqueue_service.py (2)
NotificationEnqueueService(50-294)enqueue_entry_notification(69-182)backend/services/ingestion_service.py (1)
ingest_entry(17-115)
backend/main.py (1)
backend/services/notification_scheduler.py (3)
NotificationScheduler(10-71)start(23-44)stop(46-58)
backend/services/notification_enqueue_service.py (3)
backend/services/notification_service.py (2)
NotificationService(18-501)enqueue_notification(51-121)backend/services/supabase_client.py (1)
get_supabase_client(4-9)backend/services/cache_service.py (3)
CacheService(11-301)get_notification_settings_batch(71-131)get_push_tokens_batch(172-238)
backend/services/cache_service.py (2)
backend/services/redis_client.py (1)
get_redis_client(18-59)backend/services/supabase_client.py (1)
get_supabase_client(4-9)
backend/tests/test_cache_service.py (1)
backend/services/cache_service.py (5)
CacheService(11-301)get_notification_settings(34-69)get_notification_settings_batch(71-131)get_push_tokens(133-170)get_push_tokens_batch(172-238)
backend/tests/test_notification_service_integration.py (2)
backend/services/notification_service.py (5)
NotificationService(18-501)enqueue_notification(51-121)process_queue(123-182)process_dlq(477-501)_send_notification(258-334)backend/services/notification_scheduler.py (4)
NotificationScheduler(10-71)start(23-44)stop(46-58)_process_queue_job(60-71)
backend/tests/test_notification_enqueue_service_integration.py (2)
backend/services/notification_enqueue_service.py (4)
NotificationEnqueueService(50-294)enqueue_entry_notification(69-182)_filter_recipients_by_notification_settings(226-265)_get_push_tokens_for_users(267-294)backend/services/notification_service.py (2)
NotificationService(18-501)enqueue_notification(51-121)
backend/services/notification_scheduler.py (1)
backend/services/notification_service.py (2)
NotificationService(18-501)process_queue(123-182)
backend/tests/test_notification_service.py (1)
backend/services/notification_service.py (6)
enqueue_notification(51-121)_send_notification(258-334)_handle_failure(336-421)process_queue(123-182)_log_error_to_posthog(445-475)_delete_message(423-443)
🪛 actionlint (1.7.9)
.github/workflows/test.yml
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
21-21: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 Ruff (0.14.10)
backend/routers/webhooks.py
65-68: Abstract raise to an inner function
(TRY301)
75-75: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/main.py
77-77: Use explicit conversion flag
Replace with conversion flag
(RUF010)
87-87: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/services/notification_enqueue_service.py
178-178: Consider moving this statement to an else block
(TRY300)
181-181: Use explicit conversion flag
Replace with conversion flag
(RUF010)
193-193: Consider moving this statement to an else block
(TRY300)
194-194: Do not catch blind exception: Exception
(BLE001)
195-195: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
195-195: Use explicit conversion flag
Replace with conversion flag
(RUF010)
222-222: Do not catch blind exception: Exception
(BLE001)
223-223: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
223-223: Use explicit conversion flag
Replace with conversion flag
(RUF010)
260-260: Consider moving this statement to an else block
(TRY300)
262-262: Do not catch blind exception: Exception
(BLE001)
263-263: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
263-263: Use explicit conversion flag
Replace with conversion flag
(RUF010)
290-290: Consider moving this statement to an else block
(TRY300)
292-292: Do not catch blind exception: Exception
(BLE001)
293-293: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
293-293: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/services/cache_service.py
65-65: Consider moving this statement to an else block
(TRY300)
67-67: Do not catch blind exception: Exception
(BLE001)
68-68: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
68-68: Use explicit conversion flag
Replace with conversion flag
(RUF010)
101-101: Use explicit conversion flag
Replace with conversion flag
(RUF010)
105-105: Do not catch blind exception: Exception
(BLE001)
106-106: Use explicit conversion flag
Replace with conversion flag
(RUF010)
128-128: Do not catch blind exception: Exception
(BLE001)
129-129: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
129-129: Use explicit conversion flag
Replace with conversion flag
(RUF010)
166-166: Consider moving this statement to an else block
(TRY300)
168-168: Do not catch blind exception: Exception
(BLE001)
169-169: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
169-169: Use explicit conversion flag
Replace with conversion flag
(RUF010)
200-200: Use explicit conversion flag
Replace with conversion flag
(RUF010)
204-204: Do not catch blind exception: Exception
(BLE001)
205-205: Use explicit conversion flag
Replace with conversion flag
(RUF010)
235-235: Do not catch blind exception: Exception
(BLE001)
236-236: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
236-236: Use explicit conversion flag
Replace with conversion flag
(RUF010)
266-266: Consider moving this statement to an else block
(TRY300)
268-268: Do not catch blind exception: Exception
(BLE001)
269-269: Use explicit conversion flag
Replace with conversion flag
(RUF010)
297-297: Consider moving this statement to an else block
(TRY300)
299-299: Do not catch blind exception: Exception
(BLE001)
300-300: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/services/notification_service.py
96-96: Local variable response is assigned to but never used
Remove assignment to unused variable response
(F841)
108-108: Consider moving this statement to an else block
(TRY300)
111-111: Use explicit conversion flag
Replace with conversion flag
(RUF010)
176-176: Use explicit conversion flag
Replace with conversion flag
(RUF010)
195-195: Local variable read_ct is assigned to but never used
Remove assignment to unused variable read_ct
(F841)
212-212: Local variable metadata is assigned to but never used
Remove assignment to unused variable metadata
(F841)
247-247: Use explicit conversion flag
Replace with conversion flag
(RUF010)
302-302: Consider moving this statement to an else block
(TRY300)
316-316: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
333-333: Use explicit conversion flag
Replace with conversion flag
(RUF010)
363-363: Local variable response is assigned to but never used
Remove assignment to unused variable response
(F841)
378-378: Use explicit conversion flag
Replace with conversion flag
(RUF010)
411-411: Use explicit conversion flag
Replace with conversion flag
(RUF010)
442-442: Use explicit conversion flag
Replace with conversion flag
(RUF010)
475-475: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/tests/test_cache_service.py
204-204: Unused function argument: mock_supabase_client
(ARG001)
backend/tests/test_notification_service_integration.py
104-104: Unused function argument: kwargs
(ARG001)
169-169: Unused function argument: kwargs
(ARG001)
245-245: Unused function argument: kwargs
(ARG001)
316-316: Unused function argument: kwargs
(ARG001)
377-377: Unused function argument: kwargs
(ARG001)
484-484: Unused function argument: args
(ARG001)
484-484: Unused function argument: kwargs
(ARG001)
522-522: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
570-570: Unused function argument: kwargs
(ARG001)
backend/services/redis_client.py
54-54: Consider moving this statement to an else block
(TRY300)
56-56: Do not catch blind exception: Exception
(BLE001)
57-57: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/services/notification_scheduler.py
71-71: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/tests/test_notification_service.py
342-342: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (35)
backend/.gitignore (1)
29-30: Verify that this change aligns with the PR objectives.The addition of the
blogs/ignore pattern appears unrelated to the stated PR objective of adding a notification system. Confirm that:
- This change is intentional and not an accidental inclusion
- It's safe to ignore the entire
blogs/directory and that no tracked files should be committed from there- The change should be documented in the PR description if it's a legitimate side change
backend/services/notification_enqueue_service.py (6)
10-47: LGTM!The TypedDict definitions provide useful type hints for data structures from external sources, and appropriately use
total=Falseto handle potentially missing fields from database queries.
53-67: LGTM!The initialization properly sets up all required dependencies and logs completion for observability.
69-182: LGTM!The method implements a robust enqueueing workflow with proper validation, early returns for edge cases, and comprehensive error handling. The logic correctly distinguishes between "no action needed" (returns
True) and actual failures (returnsFalse).
198-224: LGTM!The bidirectional friendship query correctly handles both relationship directions and maps results to a consistent structure. The fail-safe error handling (empty list) is appropriate.
226-265: LGTM!The filtering logic implements a sensible fail-open strategy: users without settings or with missing notification_type keys are included by default. This ensures notifications aren't silently dropped due to missing configuration.
267-294: LGTM!The method efficiently collects and flattens push tokens using batch operations, with appropriate fail-safe behavior (empty list) on errors.
backend/routers/webhooks.py (3)
5-5: LGTM!The module-level instantiation is appropriate for this stateless service and avoids repeated initialization overhead.
Also applies to: 16-16
32-42: LGTM!The docstring accurately documents the enhanced webhook behavior, including the notification enqueueing step and its failure handling semantics.
61-81: LGTM!The webhook flow correctly prioritizes ingestion (must succeed) while treating notification enqueueing as best-effort (failures logged but don't abort). The async/await usage is correct, and the broad exception catch is appropriate for preventing notification failures from breaking the webhook.
backend/services/notification_service.py (6)
21-49: LGTM!The initialization properly sets up all required clients and configuration, with optional PostHog integration that gracefully degrades when not configured.
123-182: LGTM!The queue processing implementation correctly uses concurrent message processing with asyncio.gather, appropriate visibility timeout, and comprehensive stats tracking.
258-334: LGTM!The notification delivery implements robust retry logic with exponential backoff and jitter for rate-limit handling. The use of
random.uniform()for jitter is appropriate here (not a security context), so the S311 static analysis hint is a false positive.
336-421: LGTM!The failure handling correctly implements the DLQ pattern with configurable retry limits. PostHog logging for discarded messages provides important observability for permanently failed notifications.
423-443: LGTM!The message deletion correctly re-raises exceptions to allow callers to handle delete failures appropriately.
477-501: LGTM!The DLQ processing elegantly reuses the main queue processing logic by temporarily swapping queue names. The try-finally ensures the original queue name is always restored.
.github/workflows/test.yml (1)
32-50: LGTM!The test execution is well-configured with comprehensive environment variables covering all service dependencies. The pytest flags provide good visibility while keeping output manageable.
backend/services/notification_scheduler.py (1)
1-71: LGTM! Well-structured scheduler implementation.The
NotificationSchedulerclass is cleanly implemented with proper guards against double-start/stop, appropriate logging, and exception handling in the background job. The 15-minute interval withreplace_existing=Trueensures idempotent job registration.One minor style suggestion from static analysis: consider using
{e!r}instead ofstr(e)on line 71 for more explicit exception formatting, but this is optional.backend/services/cache_service.py (3)
11-32: LGTM! Well-designed cache service with graceful degradation.The
CacheServiceprovides a clean abstraction over Redis caching with Supabase fallback. The lazy-loading pattern and TTL-based caching are appropriate for notification settings and push tokens.
71-131: Good batch optimization with proper fallback handling.The batch methods efficiently use Redis
mgetand properly handle partial cache hits by falling back to Supabase only for uncached entries. The JSON parsing error handling that treats corrupt cache entries as misses is a resilient approach.
159-164: Good practice: Caching empty lists to prevent cache stampede.Caching empty token lists (lines 162-164) is a good pattern that prevents repeated Supabase queries for users without push tokens. This avoids the "cache stampede" problem for negative lookups.
backend/tests/test_notification_service_integration.py (1)
19-56: LGTM! Well-configured test fixture with proper mocking.The fixture correctly patches Supabase client, PostHog, and notification settings. The configuration aligns with the
NotificationServiceinitialization requirements.backend/tests/test_cache_service.py (3)
16-78: LGTM! Well-structured test fixtures with proper mocking.The fixtures correctly set up mocked Redis and Supabase clients, with separate fixtures for Redis-enabled and Redis-disabled scenarios. The TTL configuration is properly patched.
120-230: Good coverage of cache hit/miss and error scenarios.The tests thoroughly cover cache hits, cache misses with Supabase fallback, Redis errors, and the Redis-unavailable case. The assertions verify both the return values and the expected client interactions.
484-504: Good test for not-found handling.This test correctly verifies that
Noneresults from Supabase are not cached, preventing stale negative cache entries that could mask newly created settings.backend/tests/test_notification_service.py (3)
16-67: LGTM! Well-configured test fixtures.The fixtures properly mock Supabase client, PostHog, and notification settings. The service is correctly instantiated with the mocked dependencies.
70-106: Good test coverage for enqueue_notification.The test properly validates the RPC call structure, queue name, and message payload fields.
179-230: Good test coverage for rate limit retry logic.The tests properly validate both the retry-with-backoff behavior on 429 errors and the max-retries-exceeded case. This ensures the exponential backoff implementation works correctly.
backend/services/redis_client.py (2)
7-13: Good pattern for optional dependency handling.The try/except block around the Redis import with a fallback flag allows the application to gracefully degrade when Redis is not installed.
40-46: No action needed. TheConnectionPool.from_url()implementation is correct. The defaultREDIS_URLconfiguration (redis://localhost:6379) does not embed a password, andREDIS_PASSWORDis passed as a separate optional environment variable. This pattern is standard and properly supported byConnectionPool.from_url(), which will use the separate password parameter when provided.backend/config.py (2)
8-28: LGTM! Well-designed helper function.The
_get_int_envhelper provides safe integer parsing with proper error handling and a clear fallback strategy. The comprehensive docstring is helpful for maintainability.
48-50: Well-structured configuration additions.The new PostHog and Redis configuration sections are properly structured with sensible defaults. Good use of
Optional[str]forREDIS_PASSWORDto reflect its possible absence, and correct usage of_get_int_envforREDIS_DBandREDIS_CACHE_TTL.Also applies to: 59-63
backend/tests/test_notification_enqueue_service_integration.py (3)
17-38: LGTM! Clean fixture definitions.The
mock_supabase_clientandmock_redis_clientfixtures are straightforward and well-documented, providing appropriate mocks for integration testing.
41-94: Complex but necessary fixture setup.The
notification_enqueue_servicefixture orchestrates multiple patches and manual wiring to create a testable service instance. While the complexity is justified for integration testing, the manual attribute assignments (lines 90-93) create coupling between test setup and service internals. This is acceptable for integration tests but may require updates if the service structure changes.
153-406: Excellent test coverage of integration scenarios.The test suite comprehensively covers critical integration paths:
- Cache miss with Supabase fallback (lines 153-228)
- Redis failure resilience (lines 231-292)
- Operation without Redis (lines 295-354)
- Recipient filtering by notification settings (lines 357-382)
- Batch token retrieval (lines 385-406)
Each test follows a clear arrange-act-assert pattern and includes appropriate assertions to verify both the happy path and edge cases. The verification at line 228 confirms cache population after fallback, and line 290 confirms the service continues to function when Redis fails.
| with patch("asyncio.sleep", side_effect=mock_sleep): | ||
| result = await notification_service._send_notification( | ||
| title="Test", | ||
| body="Test", | ||
| recipients=["token"], | ||
| priority="default", | ||
| retry_count=0, | ||
| max_retries=3 | ||
| ) |
There was a problem hiding this comment.
Remove unused result variable.
The result variable on line 522 is assigned but never used. Either remove the assignment or add an assertion on the result.
🔎 Proposed fix
with patch("asyncio.sleep", side_effect=mock_sleep):
- result = await notification_service._send_notification(
+ await notification_service._send_notification(
title="Test",
body="Test",
recipients=["token"],
priority="default",
retry_count=0,
max_retries=3
)📝 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.
| with patch("asyncio.sleep", side_effect=mock_sleep): | |
| result = await notification_service._send_notification( | |
| title="Test", | |
| body="Test", | |
| recipients=["token"], | |
| priority="default", | |
| retry_count=0, | |
| max_retries=3 | |
| ) | |
| with patch("asyncio.sleep", side_effect=mock_sleep): | |
| await notification_service._send_notification( | |
| title="Test", | |
| body="Test", | |
| recipients=["token"], | |
| priority="default", | |
| retry_count=0, | |
| max_retries=3 | |
| ) |
🧰 Tools
🪛 Ruff (0.14.10)
522-522: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🤖 Prompt for AI Agents
In backend/tests/test_notification_service_integration.py around lines 521 to
529 the local variable result is assigned from await
notification_service._send_notification but never used; remove the unused
assignment by awaiting the call directly (await
notification_service._send_notification(...)) or, if the test should validate
the returned value, replace the assignment with an appropriate assertion on
result (e.g., assert result == <expected>) and remove any unused variable
otherwise.
…lities - Updated redis from 5.0.1 to 7.1.0 (fixes CVE-2024-46981, CVE-2024-51737, CVE-2024-51480, CVE-2024-55656) - Updated posthog from 3.4.0 to 7.4.2 (no known vulnerabilities) - All tests pass with updated versions - No breaking changes detected in codebase
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.