chore: consolidate load_dotenv, remove decorative headers, fix emojis (OPE-86, OPE-74)#255
Conversation
|
@DevanshuNEU is attempting to deploy a commit to the Dev's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughMain changes: dotenv loading moved to application startup in Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/sentry.py (1)
162-175:⚠️ Potential issue | 🟡 MinorDuplicate
passat line 175 is dead code.The
except ImportErrorblock incapture_http_exceptionhas two consecutivepassstatements.🐛 Proposed fix
except ImportError: pass - pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/sentry.py` around lines 162 - 175, The except ImportError block in capture_http_exception contains a duplicated dead `pass`; remove the redundant `pass` so the exception handler has only a single no-op or consider logging the import failure. Update the function capture_http_exception to keep just one `pass` (or replace with minimal logging) inside the `except ImportError:` block to eliminate the duplicate statement.
🧹 Nitpick comments (2)
backend/services/sentry.py (1)
120-128:sentry_sdk.push_scope()is deprecated in sentry-sdk 2.0 — prefernew_scope()orisolation_scope().The old APIs will continue to work in 2.0, but Sentry recommends migrating to their new counterparts as soon as possible since they will be removed in the next major release. The same pattern appears at lines 137–142 and lines 166–173.
Since the three functions using
push_scope()are already markedDEPRECATEDthemselves, you can address this as part of the broader migration to theobservabilitymodule, or update them now to silence the deprecation warnings in the interim.♻️ Suggested replacement (same pattern applies to `capture_message` and `capture_http_exception`)
- with sentry_sdk.push_scope() as scope: + with sentry_sdk.new_scope() as scope: for key, value in extra_context.items(): scope.set_extra(key, value) sentry_sdk.capture_exception(error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/sentry.py` around lines 120 - 128, Replace deprecated sentry_sdk.push_scope() usage with the new API by calling sentry_sdk.new_scope() (or sentry_sdk.isolation_scope()) where the functions capture_exception, capture_message, and capture_http_exception set extra_context and capture events; implement a small compatibility check that prefers new_scope/isolation_scope but falls back to push_scope if the installed sentry-sdk lacks the new method, and keep the existing loop over extra_context to set scope.set_extra(key, value) before calling sentry_sdk.capture_exception(error) (or the equivalent capture call) so behavior remains identical while silencing the deprecation warning.backend/main.py (1)
5-8: LGTM —load_dotenv()correctly placed before all service imports.Calling it here ensures every downstream import (including
sentry,config, and all routes) has the correct env vars at module-load time without relying on individual modules to bootstrap their own environment. The defaultoverride=Falsebehaviour correctly lets real environment variables (CI, container runtime) win over the.envfile.One minor advisory: the
# Do not call load_dotenv() anywhere elsecomment is convention-only. A cheap CI guard prevents regression:# In a pre-commit hook or CI step — flags any load_dotenv() call outside main.py rg -n 'load_dotenv\(\)' --type py -g '!backend/main.py'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 5 - 8, Add a small CI/pre-commit check that fails the build if any file other than backend/main.py calls load_dotenv(); implement this by scanning the repository for the symbol load_dotenv() (e.g., using ripgrep or a simple Python script) and erroring when matches are found outside backend/main.py, and wire that check into the CI job or pre-commit configuration so the convention in main.py is enforced automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/services/sentry.py`:
- Around line 162-175: The except ImportError block in capture_http_exception
contains a duplicated dead `pass`; remove the redundant `pass` so the exception
handler has only a single no-op or consider logging the import failure. Update
the function capture_http_exception to keep just one `pass` (or replace with
minimal logging) inside the `except ImportError:` block to eliminate the
duplicate statement.
---
Nitpick comments:
In `@backend/main.py`:
- Around line 5-8: Add a small CI/pre-commit check that fails the build if any
file other than backend/main.py calls load_dotenv(); implement this by scanning
the repository for the symbol load_dotenv() (e.g., using ripgrep or a simple
Python script) and erroring when matches are found outside backend/main.py, and
wire that check into the CI job or pre-commit configuration so the convention in
main.py is enforced automatically.
In `@backend/services/sentry.py`:
- Around line 120-128: Replace deprecated sentry_sdk.push_scope() usage with the
new API by calling sentry_sdk.new_scope() (or sentry_sdk.isolation_scope())
where the functions capture_exception, capture_message, and
capture_http_exception set extra_context and capture events; implement a small
compatibility check that prefers new_scope/isolation_scope but falls back to
push_scope if the installed sentry-sdk lacks the new method, and keep the
existing loop over extra_context to set scope.set_extra(key, value) before
calling sentry_sdk.capture_exception(error) (or the equivalent capture call) so
behavior remains identical while silencing the deprecation warning.
OPE-86: Consolidated load_dotenv() to single call in main.py. Removed from: dependencies.py, cache.py, indexer_optimized.py, supabase_service.py. Env vars are now loaded once before any service imports. OPE-74: Removed 95 decorative header lines across 19 files. Converted '# ===== SECTION =====' to plain '# Section' comments. Replaced 4 emoji print statements in sentry.py with [INFO]/[OK]/[WARN]. Files changed: 19 Lines removed: 155 (decorative noise) Lines added: 30 (load_dotenv in main.py + simplified comments) 284 tests pass.
DevanshuNEU
force-pushed
the
chore/cleanup-dotenv-headers-emojis
branch
from
c649e25 to
37f4997
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/tests/test_multi_tenancy.py (1)
188-202:⚠️ Potential issue | 🟡 MinorRemove the duplicate
verify_repo_accesscall and assertion.Lines 199–202 repeat the identical call site and assertion already performed on lines 192–197 (same mock value, same args, same expected status code). The second block is dead test code that adds no coverage.
🐛 Proposed fix
with patch.object(repo_manager, 'verify_ownership', return_value=False): with pytest.raises(HTTPException) as exc_info: verify_repo_access("repo-user2-a", "user-1") assert exc_info.value.status_code == 404 - - with pytest.raises(HTTPException) as exc_info: - verify_repo_access("repo-user2-a", "user-1") - - assert exc_info.value.status_code == 404🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_multi_tenancy.py` around lines 188 - 202, The test contains a duplicated assertion block for verify_repo_access; remove the second with pytest.raises(HTTPException) block (the repeated call to verify_repo_access("repo-user2-a", "user-1") and its assert on exc_info.value.status_code == 404) so the test only calls verify_repo_access once under the patched repo_manager.verify_ownership=False scenario; keep the initial patch and the first pytest.raises/assert and delete the redundant duplicate to eliminate dead test code.backend/services/style_analyzer.py (4)
287-297:⚠️ Potential issue | 🟡 Minor
load_from_cachereturn type annotation is wrong — can returnNone.The signature declares
-> Dictbut returnsNoneat line 297 when there is no cached data. Callers typed againstDictwill get aNoneat runtime, causingAttributeError/TypeErroron any downstream dict access.🐛 Proposed fix
- def load_from_cache(self, repo_id: str) -> Dict: + def load_from_cache(self, repo_id: str) -> Optional[Dict]:Also add
Optionalto the import at line 6:-from typing import Dict, List, Set +from typing import Dict, List, Optional, Set🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/style_analyzer.py` around lines 287 - 297, The return type for load_from_cache is incorrect—update the function signature to return an Optional[Dict] (e.g., def load_from_cache(self, repo_id: str) -> Optional[Dict]:) and add Optional to the typing imports (e.g., from typing import Dict, Optional); keep the existing return of None when cached_styles is falsy and ensure cached_styles is used as a Dict when present (refer to the load_from_cache function and cached_styles variable).
267-285:⚠️ Potential issue | 🟠 Major
save_to_cachesilently no-ops when called withanalyze_repository_styleoutput — data structure mismatch.
save_to_cacheiteratesstyle_data.get("languages", {})(line 274), butanalyze_repository_stylereturns"language_distribution"(not"languages") as the language-keyed dict. The default{}makes the loop a silent no-op, so nothing is ever written to Supabase.Additionally:
save_to_cachereads per-language keys"naming_conventions","async_usage","type_hints"from each entry, butanalyze_repository_stylestores these at the top level, not per-language.load_from_cachereturns{"languages": ..., "top_imports": ..., "patterns": ...}— a different shape fromanalyze_repository_style— so callers receive inconsistent data structures depending on whether data is live vs. cached.The
save_to_cache/load_from_cachepair needs to be reconciled with the structure produced byanalyze_repository_style, oranalyze_repository_styleneeds to emit the"languages"format that the cache layer expects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/style_analyzer.py` around lines 267 - 285, The save/load cache shape is inconsistent with analyze_repository_style: update save_to_cache to read language entries from style_data.get("language_distribution", {}) (not "languages") and when calling db.upsert_code_style supply the expected per-language payload by pulling per-language values (naming_conventions, async_usage, type_hints) either from the language entry if present or from the top-level keys on style_data (fallback), and include top_imports and patterns; then update load_from_cache to return the same structure analyze_repository_style produces (i.e., "language_distribution" plus top_imports and patterns) so callers receive a consistent shape across analyze_repository_style, save_to_cache, load_from_cache and ensure db.upsert_code_style usage matches the new payload format.
140-140:⚠️ Potential issue | 🟡 MinorOperator precedence bug causes false positives for JS/TS type-hint detection.
andbinds tighter thanor, so line 140 evaluates as:(': ' in source_code and 'interface' in source_code) or ('type ' in source_code)Any JS/TS file containing the token
type(a TypeScript keyword, but also routine in JSDoc comments and string literals) will returnTrueregardless of whether:is present, making the:check meaningless.🐛 Proposed fix
- return ': ' in source_code and 'interface' in source_code or 'type ' in source_code + return ': ' in source_code and ('interface' in source_code or 'type ' in source_code)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/style_analyzer.py` at line 140, The current boolean expression returns True whenever "type " appears even if ": " is absent due to operator precedence; change the return to require ": " plus either "interface" or "type " by grouping the or: replace the existing return that checks source_code with a grouped expression like return ': ' in source_code and ('interface' in source_code or 'type ' in source_code) so both tokens are required (update the expression using the same variable name source_code where the original return appears).
23-27:⚠️ Potential issue | 🟠 MajorFix type hints, boolean operator precedence, and key mismatch in cache operations.
Missing return type annotation (line 267):
save_to_cachelacks-> Noneperbackend/**/*.pyguidelines.Incorrect return type annotation (line 287):
load_from_cachereturnsNoneon line 297 but is annotated-> Dict. Should beOptional[Dict].Boolean operator precedence bug (line 140):
return ': ' in source_code and 'interface' in source_code or 'type ' in source_codeDue to Python precedence (
and>or), this evaluates as(': ' in source_code and 'interface' in source_code) or ('type ' in source_code). Any file containingtypereturnsTrueregardless of the colon check. Likely intent:': ' in source_code and ('interface' in source_code or 'type ' in source_code).Key mismatch in cache save/load (line 274):
save_to_cacheiteratesstyle_data.get("languages", {}), butanalyze_repository_stylereturns"language_distribution"(line 246), not"languages". The loop executes zero times — nothing is ever cached. Use the correct key from the returned structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/style_analyzer.py` around lines 23 - 27, Update three type hints, fix a boolean-precedence bug, and correct the cache key: add "-> None" return annotation to save_to_cache, change load_from_cache's return annotation to "-> Optional[Dict]" and ensure it returns None on cache-miss; in the boolean expression (the function that inspects source_code for TypeScript interface/type), parenthesize so it reads ': ' in source_code and ('interface' in source_code or 'type ' in source_code); and in save_to_cache iterate the correct key "language_distribution" (not "languages") to match what analyze_repository_style returns so languages are actually cached.
🧹 Nitpick comments (2)
backend/tests/test_multi_tenancy.py (1)
70-119: Misleading test names — bodies only check signatures, not return values.
test_get_repository_with_owner_returns_none_for_wrong_user,test_verify_repo_ownership_returns_false_for_wrong_user, andtest_verify_repo_ownership_returns_true_for_ownerall promise behavioral assertions in their names but only inspect method signatures viainspect. This can mislead future contributors into believing ownership logic is tested when it is not.Consider renaming them to reflect what they actually verify, e.g.
test_get_repository_with_owner_has_correct_signature,test_verify_repo_ownership_has_correct_signature, etc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_multi_tenancy.py` around lines 70 - 119, Rename the three misleading tests so their names reflect that they only check method signatures rather than behavior; for example change test_get_repository_with_owner_returns_none_for_wrong_user to test_get_repository_with_owner_has_correct_signature, test_verify_repo_ownership_returns_false_for_wrong_user to test_verify_repo_ownership_has_correct_signature, and test_verify_repo_ownership_returns_true_for_owner to test_verify_repo_ownership_exists_with_correct_signature; update any related assertions/comments to mention SupabaseService.get_repository_with_owner and SupabaseService.verify_repo_ownership signature checks (repo_id, user_id params and bool return annotation) so intent is clear.backend/services/style_analyzer.py (1)
267-267: Missing return type annotation onsave_to_cache.♻️ Proposed fix
- def save_to_cache(self, repo_id: str, style_data: Dict): + def save_to_cache(self, repo_id: str, style_data: Dict) -> None:As per coding guidelines, type hints are required on all function signatures in the Python backend.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/style_analyzer.py` at line 267, The method save_to_cache(repo_id: str, style_data: Dict) is missing a return type annotation; update its signature to include an explicit return type (e.g., -> None) so it conforms to project typing rules, and ensure any callers or overrides still match the annotated signature; reference the save_to_cache method and its parameters repo_id and style_data when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/services/style_analyzer.py`:
- Around line 287-297: The return type for load_from_cache is incorrect—update
the function signature to return an Optional[Dict] (e.g., def
load_from_cache(self, repo_id: str) -> Optional[Dict]:) and add Optional to the
typing imports (e.g., from typing import Dict, Optional); keep the existing
return of None when cached_styles is falsy and ensure cached_styles is used as a
Dict when present (refer to the load_from_cache function and cached_styles
variable).
- Around line 267-285: The save/load cache shape is inconsistent with
analyze_repository_style: update save_to_cache to read language entries from
style_data.get("language_distribution", {}) (not "languages") and when calling
db.upsert_code_style supply the expected per-language payload by pulling
per-language values (naming_conventions, async_usage, type_hints) either from
the language entry if present or from the top-level keys on style_data
(fallback), and include top_imports and patterns; then update load_from_cache to
return the same structure analyze_repository_style produces (i.e.,
"language_distribution" plus top_imports and patterns) so callers receive a
consistent shape across analyze_repository_style, save_to_cache, load_from_cache
and ensure db.upsert_code_style usage matches the new payload format.
- Line 140: The current boolean expression returns True whenever "type " appears
even if ": " is absent due to operator precedence; change the return to require
": " plus either "interface" or "type " by grouping the or: replace the existing
return that checks source_code with a grouped expression like return ': ' in
source_code and ('interface' in source_code or 'type ' in source_code) so both
tokens are required (update the expression using the same variable name
source_code where the original return appears).
- Around line 23-27: Update three type hints, fix a boolean-precedence bug, and
correct the cache key: add "-> None" return annotation to save_to_cache, change
load_from_cache's return annotation to "-> Optional[Dict]" and ensure it returns
None on cache-miss; in the boolean expression (the function that inspects
source_code for TypeScript interface/type), parenthesize so it reads ': ' in
source_code and ('interface' in source_code or 'type ' in source_code); and in
save_to_cache iterate the correct key "language_distribution" (not "languages")
to match what analyze_repository_style returns so languages are actually cached.
In `@backend/tests/test_multi_tenancy.py`:
- Around line 188-202: The test contains a duplicated assertion block for
verify_repo_access; remove the second with pytest.raises(HTTPException) block
(the repeated call to verify_repo_access("repo-user2-a", "user-1") and its
assert on exc_info.value.status_code == 404) so the test only calls
verify_repo_access once under the patched repo_manager.verify_ownership=False
scenario; keep the initial patch and the first pytest.raises/assert and delete
the redundant duplicate to eliminate dead test code.
---
Nitpick comments:
In `@backend/services/style_analyzer.py`:
- Line 267: The method save_to_cache(repo_id: str, style_data: Dict) is missing
a return type annotation; update its signature to include an explicit return
type (e.g., -> None) so it conforms to project typing rules, and ensure any
callers or overrides still match the annotated signature; reference the
save_to_cache method and its parameters repo_id and style_data when making the
change.
In `@backend/tests/test_multi_tenancy.py`:
- Around line 70-119: Rename the three misleading tests so their names reflect
that they only check method signatures rather than behavior; for example change
test_get_repository_with_owner_returns_none_for_wrong_user to
test_get_repository_with_owner_has_correct_signature,
test_verify_repo_ownership_returns_false_for_wrong_user to
test_verify_repo_ownership_has_correct_signature, and
test_verify_repo_ownership_returns_true_for_owner to
test_verify_repo_ownership_exists_with_correct_signature; update any related
assertions/comments to mention SupabaseService.get_repository_with_owner and
SupabaseService.verify_repo_ownership signature checks (repo_id, user_id params
and bool return annotation) so intent is clear.
…ate test style_analyzer.py: 1. Boolean precedence bug in _check_type_hints: 'type ' alone returned True without ': ' due to and/or precedence. Added parentheses. 2. Cache key mismatch: save_to_cache read 'languages' but analyze_repository_style returns 'language_distribution'. Caching was silently broken (iterated empty dict). Fixed key + load shape. 3. load_from_cache return type: was -> Dict, returns None. Fixed to -> Optional[Dict], added Optional to imports. 4. save_to_cache: added -> None return annotation. test_multi_tenancy.py: 5. Removed duplicate assertion block in test_verify_repo_access_raises_404_for_wrong_user (exact same verify_repo_access call repeated, dead code). 284 tests pass.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/style_analyzer.py (1)
274-281:⚠️ Potential issue | 🟠 Major
datanever has the analysis keys —naming_convention,async_usage, andtype_hintsare always cached asNone.
analyze_repository_style()returnslanguage_distributionwith this shape per language:{"count": int, "percentage": str}
data.get("naming_conventions"),data.get("async_usage"), anddata.get("type_hints")therefore always resolve toNone. The aggregate analysis data lives at the top level ofstyle_data, not nested inside each language entry.Downstream in
load_from_cache,style.get("naming_convention", {})also returnsNone(not{}) when the DB column holds an explicitNone, so consumers getNoneinstead of dict for these fields.The aggregate
naming_conventionsandpatternsdata should be read from the top-levelstyle_datainstead:🐛 Proposed fix
for language, data in style_data.get("language_distribution", {}).items(): analysis = { - "naming_convention": data.get("naming_conventions"), - "async_usage": data.get("async_usage"), - "type_hints": data.get("type_hints"), + "naming_convention": style_data.get("naming_conventions"), + "async_usage": style_data.get("patterns", {}).get("async_percentage"), + "type_hints": style_data.get("patterns", {}).get("typed_percentage"), "common_imports": style_data.get("top_imports", []), "patterns": style_data.get("patterns", {}) }Note: this stores the same aggregate stats for every language row. A complete fix would require
analyze_repository_styleto produce per-language breakdowns, but at minimum this avoids silentNonewrites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/style_analyzer.py` around lines 274 - 281, The loop building per-language "analysis" incorrectly reads aggregate fields from each language's `data` (in the loop within for language, data in style_data.get("language_distribution", {}).items()) so naming_convention, async_usage, and type_hints become None; change the construction of the `analysis` dict in that loop to take these aggregates from the top-level `style_data` (e.g. style_data.get("naming_conventions") or {} , style_data.get("async_usage") or {} , style_data.get("type_hints") or {} ), keep common_imports from style_data.get("top_imports", []), and read patterns from style_data.get("patterns", {}) so you store dicts (not None) and avoid writing None into the DB that breaks load_from_cache (which uses style.get("naming_convention", {})).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/services/style_analyzer.py`:
- Around line 274-281: The loop building per-language "analysis" incorrectly
reads aggregate fields from each language's `data` (in the loop within for
language, data in style_data.get("language_distribution", {}).items()) so
naming_convention, async_usage, and type_hints become None; change the
construction of the `analysis` dict in that loop to take these aggregates from
the top-level `style_data` (e.g. style_data.get("naming_conventions") or {} ,
style_data.get("async_usage") or {} , style_data.get("type_hints") or {} ), keep
common_imports from style_data.get("top_imports", []), and read patterns from
style_data.get("patterns", {}) so you store dicts (not None) and avoid writing
None into the DB that breaks load_from_cache (which uses
style.get("naming_convention", {})).
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
1 participant
What
Three CLAUDE.md compliance fixes bundled into one cleanup PR. Pure noise removal -- no behavior changes.
Changes
OPE-86: Consolidate load_dotenv()
load_dotenv()was called in 4 separate files (dependencies.py, cache.py, indexer_optimized.py, supabase_service.py). Now called once at the top of main.py before any service imports. Prevents import-order-dependent env var issues.OPE-74: Remove decorative ASCII headers
Removed
# ========and# --------section dividers from 13 files. CLAUDE.md says 'no decorative headers or ASCII art.' Plain section comments retained where they add context.OPE-74: Fix emojis in sentry.py
Replaced 4 emoji print statements with plain text markers (
[INFO],[OK],[WARN]). CLAUDE.md says 'NO emojis anywhere.'Note: Discord webhook embed titles in feedback.py intentionally kept -- those display in Discord's UI where emojis are standard.
Impact
Net -104 lines. 16 files touched, all deletions except 9 lines added to main.py for load_dotenv.
284 tests pass.
Closes OPE-86, OPE-74
Summary by CodeRabbit