Jules was unable to complete the task in time. Please review the work…#67
Jules was unable to complete the task in time. Please review the work…#67
Conversation
… done so far and provide feedback for Jules to continue.
Reviewer's GuideThis PR restructures core NLP and filter modules for dynamic resource handling, persistent in-memory DB management, and more robust action‐item extraction, while modernizing the test suite with consistent AsyncMock usage, correct patch targets, and a session‐scoped DB fixture. Class diagram for updated NLPEngine resource and model loadingclassDiagram
class NLPEngine {
- sentiment_model_path: str
- topic_model_path: str
- intent_model_path: str
- urgency_model_path: str
- stop_words: set
- sentiment_model
- topic_model
- intent_model
- urgency_model
+ __init__()
+ _load_model(path)
+ _analyze_topic_keyword(text)
}
Class diagram for SmartFilterManager persistent DB connectionclassDiagram
class SmartFilterManager {
- db_path: str
- conn: sqlite3.Connection | None
+ __init__(db_path: str = "smart_filters.db")
+ _get_db_connection() : sqlite3.Connection
+ _close_db_connection(conn: sqlite3.Connection)
+ _db_execute(query: str, params: tuple)
+ _db_fetchone(query: str, params: tuple)
+ _db_fetchall(query: str, params: tuple)
+ _init_filter_db()
+ close()
}
Class diagram for ActionItemExtractor with enhanced resource checks and regexclassDiagram
class ActionItemExtractor {
- action_keywords_regex: re.Pattern
- due_date_regex: re.Pattern
+ __init__()
}
Class diagram for GmailService fallback analysis outputclassDiagram
class GmailService {
+ _get_basic_fallback_analysis_structure(error_message: str) Dict[str, Any]
}
class FallbackAnalysisOutput {
- error: str
- topic: str
- sentiment: str
- intent: str
- urgency: str
- confidence: float
- keywords: list
- categories: list
- reasoning: str
- suggested_labels: list
- risk_flags: list
- action_items: list
}
GmailService --> FallbackAnalysisOutput
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis update introduces enhanced error handling and resource verification for NLTK in the action item extractor, refines model path and stopword management in the NLP engine, and ensures consistent fallback structures in Gmail service. It also standardizes async mocking in API tests, improves database connection management for smart filters, and expands test coverage and reliability. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant App as FastAPI App
participant DB as Mocked DB Session
Test->>App: Send API request (e.g., category, dashboard, email)
App->>DB: Use dependency override to get mock DB session
DB-->>App: Return mock results or raise async error
App-->>Test: Return API response
sequenceDiagram
participant Extractor as ActionItemExtractor
participant NLTK as NLTK Resources
Extractor->>NLTK: Check for required resources
alt Resource missing
Extractor->>NLTK: Download resource
NLTK-->>Extractor: Resource available
else All present
Extractor-->>Extractor: Proceed
end
Extractor->>Extractor: Apply regex for action keywords and due dates
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @MasumRab - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `server/python_nlp/smart_filters.py:54` </location>
<code_context>
+ self.conn = None # For persistent in-memory connection
+
+ if self.db_path == ":memory:":
+ self.conn = sqlite3.connect(":memory:")
+ self.conn.row_factory = sqlite3.Row
+
</code_context>
<issue_to_address>
Make in-memory SQLite connection thread-safe if shared across threads
Consider setting check_same_thread=False when creating the connection, or use a separate connection for each thread to avoid threading errors.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if self.db_path == ":memory:":
self.conn = sqlite3.connect(":memory:")
self.conn.row_factory = sqlite3.Row
=======
if self.db_path == ":memory:":
# Allow the in-memory SQLite connection to be shared across threads
self.conn = sqlite3.connect(":memory:", check_same_thread=False)
self.conn.row_factory = sqlite3.Row
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if self.db_path == ":memory:": | ||
| self.conn = sqlite3.connect(":memory:") | ||
| self.conn.row_factory = sqlite3.Row |
There was a problem hiding this comment.
suggestion (bug_risk): Make in-memory SQLite connection thread-safe if shared across threads
Consider setting check_same_thread=False when creating the connection, or use a separate connection for each thread to avoid threading errors.
| if self.db_path == ":memory:": | |
| self.conn = sqlite3.connect(":memory:") | |
| self.conn.row_factory = sqlite3.Row | |
| if self.db_path == ":memory:": | |
| # Allow the in-memory SQLite connection to be shared across threads | |
| self.conn = sqlite3.connect(":memory:", check_same_thread=False) | |
| self.conn.row_factory = sqlite3.Row |
| self.mock_db_instance.get_all_categories.assert_called_once() | ||
|
|
||
| async def async_raise_db_connection_error(self, *args, **kwargs): | ||
| raise Exception("Database connection error") |
There was a problem hiding this comment.
issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)
Explanation
If a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the built-in exceptions of the standard library.
- Define your own error class that subclasses
Exception.
So instead of having code raising Exception or BaseException like
if incorrect_input(value):
raise Exception("The input is incorrect")you can have code raising a specific error like
if incorrect_input(value):
raise ValueError("The input is incorrect")or
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")| mock_db_manager.create_category.assert_called_once_with(validated_category_data) | ||
|
|
||
| async def async_raise_db_write_error(self, *args, **kwargs): | ||
| raise Exception("Database write error") |
There was a problem hiding this comment.
issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)
Explanation
If a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the built-in exceptions of the standard library.
- Define your own error class that subclasses
Exception.
So instead of having code raising Exception or BaseException like
if incorrect_input(value):
raise Exception("The input is incorrect")you can have code raising a specific error like
if incorrect_input(value):
raise ValueError("The input is incorrect")or
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")| mock_db_manager.get_dashboard_stats.assert_called_once() # Changed to mock_db_manager | ||
|
|
||
| async def async_raise_db_error(self, *args, **kwargs): | ||
| raise Exception("DB error") |
There was a problem hiding this comment.
issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)
Explanation
If a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the built-in exceptions of the standard library.
- Define your own error class that subclasses
Exception.
So instead of having code raising Exception or BaseException like
if incorrect_input(value):
raise Exception("The input is incorrect")you can have code raising a specific error like
if incorrect_input(value):
raise ValueError("The input is incorrect")or
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")| self.mock_performance_monitor.get_real_time_dashboard.assert_called_once() | ||
|
|
||
| async def async_raise_performance_monitor_error(self, *args, **kwargs): | ||
| raise Exception("Performance monitor error") |
There was a problem hiding this comment.
issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)
Explanation
If a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the built-in exceptions of the standard library.
- Define your own error class that subclasses
Exception.
So instead of having code raising Exception or BaseException like
if incorrect_input(value):
raise Exception("The input is incorrect")you can have code raising a specific error like
if incorrect_input(value):
raise ValueError("The input is incorrect")or
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")| async def async_raise_exception(self, *args, **kwargs): | ||
| # Helper for setting side_effect to an async function that raises | ||
| # Can be specific if different tests need different exception types/messages | ||
| raise Exception("Simulated database error") |
There was a problem hiding this comment.
issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)
Explanation
If a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the built-in exceptions of the standard library.
- Define your own error class that subclasses
Exception.
So instead of having code raising Exception or BaseException like
if incorrect_input(value):
raise Exception("The input is incorrect")you can have code raising a specific error like
if incorrect_input(value):
raise ValueError("The input is incorrect")or
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")|
|
||
| # Adding async helper for POST_PUT tests, can be the same if error message is generic | ||
| async def async_raise_exception(self, *args, **kwargs): | ||
| raise Exception("Simulated database error") |
There was a problem hiding this comment.
issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)
Explanation
If a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the built-in exceptions of the standard library.
- Define your own error class that subclasses
Exception.
So instead of having code raising Exception or BaseException like
if incorrect_input(value):
raise Exception("The input is incorrect")you can have code raising a specific error like
if incorrect_input(value):
raise ValueError("The input is incorrect")or
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")| if hasattr(self, 'mock_textblob_instance'): # Check if mock_textblob_instance was created in setUp | ||
| self.mock_textblob_instance.noun_phrases = ['sample phrase', 'keyword extraction'] | ||
| self.mock_textblob_instance.words = ['this', 'is', 'sample', 'phrase', 'for', 'keyword', 'extraction', 'test'] |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| text_no_keyword = "Submit the expenses by tomorrow." | ||
| actions_no_keyword = self.extractor.extract_actions(text_no_keyword) | ||
| self.assertEqual(len(actions_no_keyword), 0) # Text without keyword should not produce action | ||
|
|
||
| # Now test with a keyword | ||
| text_with_keyword = "You should Submit the expenses by tomorrow." | ||
| actions = self.extractor.extract_actions(text_with_keyword) | ||
| self.assertEqual(len(actions), 1) | ||
| self.assertEqual(actions[0]['raw_due_date_text'], "by tomorrow") | ||
| actions_with_keyword = self.extractor.extract_actions(text_with_keyword) | ||
| self.assertEqual(len(actions_with_keyword), 1) |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into method (extract-duplicate-method)
| # However, self.engine is created in setUp. If we want to test its behavior | ||
| # under different HAS_NLTK conditions, it's better to create a new instance or patch self.engine. | ||
| # For this test, we want the path where HAS_NLTK is true. | ||
| engine_for_test = NLPEngine() # This will use the patched HAS_NLTK if it's module level |
There was a problem hiding this comment.
issue (code-quality): Extract code out into method (extract-method)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
tests/test_category_api.py (1)
129-141: Remove duplicate test methodThere are two
test_create_category_db_errormethods. The first one (lines 129-141) references the oldmock_db_managerand should be removed. Keep only the second implementation (lines 145-159) which correctly usesself.mock_db_instance.Apply this diff to remove the duplicate:
- def test_create_category_db_error(self): - print("Running test_create_category_db_error") - category_data = {"name": "Error Category", "description": "Test DB error", "color": "#ABCDEF"} - mock_db_manager.create_category.side_effect = Exception("Database write error") - - response = self.client.post("/api/categories", json=category_data) - - self.assertEqual(response.status_code, 500) - data = response.json() - self.assertIn("Failed to create category", data["detail"]) - validated_category_data = CategoryCreate(**category_data).model_dump() - mock_db_manager.create_category.assert_called_once_with(validated_category_data) - async def async_raise_db_write_error(self, *args, **kwargs):Also applies to: 145-159
tests/test_dashboard_api.py (1)
5-8: Hard dependency on psycopg2 in the test suite may break CI environmentsImporting the real
psycopg2C-extension just to accessErrorforces the test environment to compile / install the full driver. Many CI runners (and most dev machines on Windows) do not have the PostgreSQL headers or a C-compiler pre-installed, so the tests will fail to start before they even reach the application code.A safer pattern is to avoid the hard import and patch the symbol where it is used:
-from psycopg2 import Error as Psycopg2Error # Import real psycopg2.Error +try: + from psycopg2 import Error as Psycopg2Error +except ModuleNotFoundError: + # Fallback stub to keep the test runner lightweight + class Psycopg2Error(Exception): + passor simply patch the symbol inside
server.python_backend.main:with patch('server.python_backend.main.psycopg2.Error', Exception): ...This keeps the test suite self-contained and eliminates the need for system-level build tooling.
tests/test_gmail_service_integration.py (1)
10-36: Async test will be silently skipped underunittest.TestCase
unittestdoes not auto-await coroutine tests on a plainTestCase; the body never executes, so assertions are never evaluated. Useunittest.IsolatedAsyncioTestCase(3.8+) or convert the test to a sync wrapper (e.g.,asyncio.run).-class TestGmailAIServiceIntegration(unittest.TestCase): +class TestGmailAIServiceIntegration(unittest.IsolatedAsyncioTestCase):No other changes are needed—the framework will await
async defmethods automatically.
♻️ Duplicate comments (1)
tests/test_dashboard_api.py (1)
86-97: Side-effect helper raises realPsycopg2Error– see earlier noteEven if
psycopg2is installed, the heavy binary dependency is unnecessary. Consider raising the stub class suggested above or patching the symbol inmainto a lightweight custom exception.
🧹 Nitpick comments (10)
tests/conftest.py (1)
29-33: Guard clause can be simplifiedInstead of nested
if / else, useapp.dependency_overrides.pop(get_db, None)to restore original state.This is optional but reduces branching.
server/python_nlp/smart_filters.py (1)
168-175: Repeatedconn.commit()inside loopYou already hold a single connection; committing once after the loop suffices:
for query in queries: - conn.execute(query) - conn.commit() + conn.execute(query) conn.commit()Micro-optimisation but noticeable on startup.
tests/test_email_api.py (1)
125-129: Duplicateasync_raise_exceptionhelper – DRY up test code.The same async helper is defined twice (here and again at lines 348-350).
Place it once at module level (or share via a fixture) to keep tests lean.server/python_nlp/nlp_engine.py (1)
71-80: Excellent error handling for NLTK resourcesThe automatic detection and download of missing NLTK resources improves robustness. The fallback to an empty set when NLTK is unavailable ensures the code continues to work in limited environments.
Consider adding a timeout to the NLTK download to prevent hanging in environments with restricted internet access:
- nltk.download('stopwords', quiet=True) + try: + nltk.download('stopwords', quiet=True, raise_on_error=True) + except Exception as e: + logger.warning(f"Failed to download stopwords: {e}")server/python_nlp/action_item_extractor.py (1)
24-42: Comprehensive NLTK resource managementExcellent implementation of resource checking and downloading. The error handling for both checking and downloading is thorough.
Since
punktandpunkt_tabare related, consider documenting why both are checked:"averaged_perceptron_tagger_eng": "taggers/averaged_perceptron_tagger_eng", # Added for explicit check - "punkt": "tokenizers/punkt" # Redundant with top-level check but good for robustness + "punkt": "tokenizers/punkt" # Legacy tokenizer, kept for backward compatibilitytests/test_dashboard_api.py (3)
26-40: Double-patchingperformance_monitor.trackis redundant
performance_monitoris already replaced with aMagicMockin the first patch.
Patchingperformance_monitor.trackagain creates a second mock that is never exercised because attribute access on the firstMagicMockwould already return a mock.-# Patch PerformanceMonitor instance used in main.py -self.mock_performance_monitor_patch = patch('server.python_backend.main.performance_monitor', autospec=True) -... -# Mock performance_monitor.track decorator to just return the function -self.track_patch = patch('server.python_backend.main.performance_monitor.track', MagicMock(side_effect=lambda func: func)) -self.track_patch.start() +perf_mon_patcher = patch('server.python_backend.main.performance_monitor', autospec=True) +self.mock_performance_monitor = perf_mon_patcher.start() +# Simplify decorator handling +self.mock_performance_monitor.track.side_effect = lambda func: func +self.addCleanup(perf_mon_patcher.stop)This removes one level of indirection, shortens teardown code, and avoids potential inconsistencies between the two mocks.
33-36:reset_mock()immediately after construction is a no-op
mock_db_manageris created on line 14; callingreset_mock()in the same setup block has no observable effect. Dropping the line improves readability.
70-77: Re-assigningget_dashboard_statsto a freshAsyncMockis unnecessaryThe attribute was already set to an
AsyncMockinsetUp; you can simply setside_effect:-mock_db_manager.get_dashboard_stats = AsyncMock() -mock_db_manager.get_dashboard_stats.side_effect = self.async_raise_db_error +mock_db_manager.get_dashboard_stats.side_effect = self.async_raise_db_errorFewer allocations and clearer intent.
tests/test_gmail_service_integration.py (2)
51-63: Testing private API reduces robustness
Both_perform_ai_analysisand_convert_to_db_formatare underscored (private). Tests that lock onto internals can break on harmless refactors. Prefer exercising the public interface (e.g.,perform_ai_analysiswrapper or service facade) and assert on side-effects/output there.
137-177: Duplicated boilerplate – consider a fixture/helper
Both test cases re-create nearly identical mockedGmailMessageobjects. Extract a reusable builder/fixture to keep tests concise and ensure future updates stay consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
deployment/run_tests.py(1 hunks)server/python_backend/main.py(1 hunks)server/python_nlp/action_item_extractor.py(2 hunks)server/python_nlp/data_strategy.py(1 hunks)server/python_nlp/gmail_integration.py(1 hunks)server/python_nlp/gmail_service.py(1 hunks)server/python_nlp/nlp_engine.py(4 hunks)server/python_nlp/smart_filters.py(6 hunks)tests/conftest.py(1 hunks)tests/test_action_item_extractor.py(2 hunks)tests/test_category_api.py(5 hunks)tests/test_dashboard_api.py(5 hunks)tests/test_email_api.py(7 hunks)tests/test_gmail_service_integration.py(3 hunks)tests/test_nlp_engine.py(8 hunks)tests/test_smart_filters.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
server/python_backend/main.py (1)
server/python_nlp/smart_filters.py (1)
apply_filters_to_email_data(1230-1270)
tests/test_smart_filters.py (1)
server/python_nlp/smart_filters.py (5)
_get_db_connection(63-70)close(177-181)_load_filter(1010-1033)_db_fetchall(101-111)evaluate_filter_performance(846-881)
tests/test_nlp_engine.py (1)
server/python_nlp/nlp_engine.py (3)
NLPEngine(52-1071)main(1074-1123)_extract_keywords(584-632)
tests/test_email_api.py (2)
server/python_backend/database.py (2)
get_email_by_id(248-259)get_all_emails(561-563)server/python_backend/main.py (2)
create_email(244-298)update_email(302-336)
tests/test_dashboard_api.py (3)
tests/test_email_api.py (1)
override_get_db(11-12)server/python_backend/main.py (1)
get_dashboard_stats(124-149)server/python_backend/performance_monitor.py (1)
get_real_time_dashboard(186-248)
🪛 Ruff (0.11.9)
tests/test_category_api.py
145-145: Redefinition of unused test_create_category_db_error from line 129
(F811)
🪛 Pylint (3.3.7)
tests/test_category_api.py
[error] 145-145: method already defined line 129
(E0102)
🔇 Additional comments (21)
server/python_nlp/data_strategy.py (1)
9-9: Import cleanup looks good
tuplewasn't referenced anywhere, so removing it trims an unused import without side-effects. 👍server/python_nlp/gmail_integration.py (1)
9-9: Redundanttupleimport correctly removedNo logic depends on the
tuplealias, so this tidy-up is safe.server/python_nlp/smart_filters.py (1)
1084-1086:performance_iduniqueness improvement 👍Including microseconds solves collision risk in fast loops. No further action.
deployment/run_tests.py (1)
68-68: Integration tests now run full suite – may double-execute
run_unit_testsandrun_integration_testsboth invokepytest tests/.
If a CI job passes--unit --integration, the exact same tests execute twice, wasting ~50 % wall-time.Options:
-def run_integration_tests(...): - command = f"{sys.executable} -m pytest tests/" +def run_integration_tests(...): + command = f"{sys.executable} -m pytest tests/integration/"or delegate to markers (
-m "integration").Clarify intent before merge.
server/python_nlp/gmail_service.py (1)
232-240: Good catch – fallback now guaranteesaction_itemskey.Adding
action_items: []to the fallback avoidsKeyErrordownstream when callers blindly assume the field exists. 👍tests/test_action_item_extractor.py (1)
76-87: Test may be brittle – “Submit the expenses” is itself an imperative verb.Depending on future tweaks to the extractor, the first assertion (
len(actions_no_keyword) == 0) could start failing, because “Submit …” is a plain imperative that many extraction heuristics will eventually catch.Consider flagging the expectation with a more neutral sentence or relaxing the assertion to “<= 1”, or add a comment explaining why this is intentionally ignored.
tests/test_email_api.py (1)
168-169: Correct patch target – nice alignment with backend rename.Updating the patch to
apply_filters_to_email_datakeeps tests in sync with production code.server/python_nlp/nlp_engine.py (2)
45-49: Good refactoring: Dynamic model path configurationMoving model paths from module-level constants to instance attributes is a solid improvement. This allows for better testability and runtime configuration through environment variables.
Also applies to: 62-69, 93-96
366-369: Good consistency improvement for topic namingThe topic normalization ensures consistent naming patterns across the system. Converting to lowercase and replacing spaces/special characters with underscores prevents issues with case sensitivity and special character handling.
Also applies to: 375-375
server/python_nlp/action_item_extractor.py (2)
8-13: Improved error handling for NLTK importsGood addition of
LookupErrorto the exception handling. This properly catches cases where NLTK is installed but required data resources are missing.
44-57: Improved regex patterns for better extractionGood improvements to the regex patterns:
- Better organization of action keywords for maintainability
- Enhanced due date detection with "on DayName" pattern
These changes will improve the accuracy of action item extraction.
tests/test_smart_filters.py (3)
22-30: Good test isolation with proper cleanupExcellent addition of explicit database cleanup in
setUpand connection cleanup intearDown. This ensures tests are independent and prevents data pollution between tests.Also applies to: 62-63
66-84: Better test approach for database initializationGood refactoring to test database initialization through actual queries rather than schema inspection. This approach:
- Tests the actual behavior users would experience
- Is less brittle to implementation changes
- Properly detects initialization failures
224-257: Corrected test expectations for accurate validationGood correction of the test data and expected metrics. The test now accurately reflects:
- True positives for emails containing "urgent"
- Proper calculation of precision, recall, and F1 score
- Zero false positive rate for this specific filter
The corrected assertions ensure the performance evaluation logic is properly validated.
tests/test_nlp_engine.py (3)
11-18: Improved mocking logic for better test reliabilityGood improvements to the mocking setup:
- Using
ENGINE_HAS_NLTKensures tests align with the actual engine's detection- Properly mocking
stopwords.wordsas a callable prevents test failuresThese changes make the tests more reliable across different environments.
Also applies to: 77-78
6-6: Correct approach for stdout capturingGood fix using
io.StringIOinstead ofmock_openfor stdout capturing. This is the proper way to capture printed output in tests, and usinggetvalue()correctly retrieves the captured string.Also applies to: 272-272, 283-283, 290-290, 305-305, 308-308, 332-332
193-193: ```shell
#!/bin/bashLocate the test file to ensure correct path
fd -t f -i test_nlp_engine.py
Search for any definitions of fallback or important words in the repo
rg -n -C2 "fallback" .
rg -n -C2 "important_words" .Inspect occurrences of 'email' in the entire codebase
rg -n -C2 "email" .
</details> <details> <summary>tests/test_category_api.py (2)</summary> `8-32`: **Excellent refactoring of mock management** Great improvement to the test structure: - Removed global mock state - Proper patching lifecycle with setUp/tearDown - Clean state for each test with reset_mock() - Correct use of AsyncMock for async methods This makes the tests more maintainable and prevents cross-test pollution. --- `61-68`: **Correct async exception handling** Good implementation of async helper methods for exception simulation. This is the proper way to test error cases with AsyncMock. Also applies to: 142-150 </details> <details> <summary>tests/test_gmail_service_integration.py (2)</summary> `3-4`: **`json` import looks good and is necessary** `_convert_to_db_format` serialises `analysisMetadata`; pulling it back with `json.loads` requires this import, so the addition is correct. --- `69-110`: **Large attribute surface on `MagicMock(spec=GmailMessage)` may mask divergence** Setting dozens of attributes that might not exist in `GmailMessage` defeats the point of using `spec` for safety. If strictness is desired, switch to `spec_set=GmailMessage`; otherwise, drop the `spec` argument to avoid fragile coupling. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| async def override_get_db_for_session(): | ||
| return mock_db | ||
|
|
There was a problem hiding this comment.
Dependency override should yield, not return
get_db in most FastAPI back-ends is an async generator (yield session).
Returning the mock directly breaks the dependency-teardown cycle and causes:
• RuntimeError: coroutine was never awaited when the overridden dependency is awaited as an async-gen.
• Suppressed fixture finalisation for db sessions that rely on yield.
Fix:
-async def override_get_db_for_session():
- return mock_db
+async def override_get_db_for_session():
+ yield mock_db📝 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.
| async def override_get_db_for_session(): | |
| return mock_db | |
| async def override_get_db_for_session(): | |
| yield mock_db |
🤖 Prompt for AI Agents
In tests/conftest.py around lines 21 to 23, the override_get_db_for_session
function incorrectly returns the mock_db instead of yielding it, which breaks
the async generator pattern expected by FastAPI dependencies. Change the
function to use 'yield mock_db' instead of 'return mock_db' to properly support
the async generator behavior and ensure correct dependency teardown and
avoidance of runtime errors.
|
|
||
| if self.db_path == ":memory:": | ||
| self.conn = sqlite3.connect(":memory:") | ||
| self.conn.row_factory = sqlite3.Row | ||
|
|
||
| self._init_filter_db() | ||
|
|
||
| # Filter categories and templates | ||
| self.filter_templates = self._load_filter_templates() | ||
| self.pruning_criteria = self._load_pruning_criteria() | ||
|
|
||
| def _get_db_connection(self) -> sqlite3.Connection: | ||
| """Get a new database connection.""" | ||
| """Get a database connection.""" | ||
| if self.conn: # Use persistent in-memory connection if it exists | ||
| return self.conn | ||
| # For file-based DBs, create a new connection each time (or manage a pool) | ||
| conn = sqlite3.connect(self.db_path) | ||
| conn.row_factory = sqlite3.Row # Access columns by name | ||
| conn.row_factory = sqlite3.Row | ||
| return conn | ||
|
|
||
| def _close_db_connection(self, conn: sqlite3.Connection): | ||
| """Close connection if it's not the persistent in-memory one.""" | ||
| if conn is not self.conn: | ||
| conn.close() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Persistent in-memory connection logic is sound but leaks for file DBs
_get_db_connection opens a new connection for every call when db_path ≠ ":memory:" and _db_execute/fetch* close it afterwards – fine.
However, long-running loops that call these helpers thousands of times will continually open/close SQLite handles which is slow.
Consider:
- Keeping a single cached connection per file DB (same pattern you applied to
":memory:"). - Or wrapping calls in a context-manager when bulk operations are needed.
Otherwise performance degradation is likely for heavy filter traffic.
🤖 Prompt for AI Agents
In server/python_nlp/smart_filters.py around lines 52 to 76, the current code
opens and closes a new SQLite connection on every call for file-based databases,
causing performance issues during heavy usage. To fix this, implement a
persistent cached connection for file-based DBs similar to the in-memory
connection by storing and reusing a single connection instance. Alternatively,
provide a context manager for bulk operations to manage connections efficiently
and avoid frequent open/close cycles.
| # Apply smart filters | ||
| filter_results = await filter_manager.apply_filters_to_email(email.dict()) | ||
| filter_results = await filter_manager.apply_filters_to_email_data(email.dict()) # Corrected method name | ||
|
|
There was a problem hiding this comment.
Awaiting a non-coroutine will raise a runtime TypeError.
SmartFilterManager.apply_filters_to_email_data is a synchronous method (see smart_filters.py).
await expects a coroutine / awaitable; in production this line will blow up once the patch-based tests are removed.
- filter_results = await filter_manager.apply_filters_to_email_data(email.dict()) # Corrected method name
+ filter_results = filter_manager.apply_filters_to_email_data(email.dict())Alternatively, convert apply_filters_to_email_data to async def if you truly need asynchronous behaviour.
📝 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.
| # Apply smart filters | |
| filter_results = await filter_manager.apply_filters_to_email(email.dict()) | |
| filter_results = await filter_manager.apply_filters_to_email_data(email.dict()) # Corrected method name | |
| # Apply smart filters | |
| filter_results = filter_manager.apply_filters_to_email_data(email.dict()) # Corrected method name |
🤖 Prompt for AI Agents
In server/python_backend/main.py around lines 255 to 257, the code incorrectly
uses 'await' on the synchronous method 'apply_filters_to_email_data' from
SmartFilterManager, which will cause a runtime TypeError. To fix this, remove
the 'await' keyword before the method call since it is not a coroutine.
Alternatively, if asynchronous behavior is required, refactor
'apply_filters_to_email_data' in smart_filters.py to be an async function and
update its implementation accordingly.
| mock_db_manager = AsyncMock() | ||
|
|
||
| async def override_get_db_dashboard(): | ||
| return mock_db_manager_dashboard | ||
| async def override_get_db(): # Standardized name | ||
| return mock_db_manager | ||
|
|
||
| app.dependency_overrides[get_db] = override_get_db_dashboard | ||
| app.dependency_overrides[get_db] = override_get_db # Standardized override | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dependency override returns a bare AsyncMock – consider yielding a context-managed mock
override_get_db is declared async yet merely returns the shared AsyncMock instance.
While FastAPI happily awaits the coroutine, nothing ensures proper teardown (e.g. closing connections) once the request scope finishes. A more idiomatic override mirrors the original get_db dependency and yields:
-async def override_get_db():
- return mock_db_manager
+async def override_get_db():
+ try:
+ yield mock_db_manager
+ finally:
+ mock_db_manager.reset_mock()This keeps the lifetime semantics identical to production code and prevents stale state from one test bleeding into another.
🤖 Prompt for AI Agents
In tests/test_dashboard_api.py around lines 14 to 20, the override_get_db
function currently returns a bare AsyncMock instance without using a context
manager or yield, which breaks the original dependency's lifetime semantics. To
fix this, refactor override_get_db to be an async generator that yields the
mock_db_manager, ensuring proper setup and teardown like the original get_db
dependency. This prevents stale state across tests by maintaining the request
scope lifecycle.
1 participant
… done so far and provide feedback for Jules to continue.
Summary by Sourcery
Standardize and improve test suites across the project, refactor the NLP engine and smart filter components for greater configurability and consistency, and update deployment scripts to run the full test suite.
Bug Fixes:
Enhancements:
Deployment:
Tests:
Chores:
Summary by CodeRabbit