Release 1.0.0 - #3
Conversation
|
Warning Review limit reached
More reviews will be available in 34 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces LLM-driven chat session title generation via Celery, adds profile management APIs for onboarding workflows, and enhances multi-language support across document generation and message translations. ChangesSession Title Generation
Profile Management and Onboarding
Multi-Language Support
Minor Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
chatbot/consumers/async_consumer.py (1)
253-255: ⚡ Quick winInconsistent error handling for missing CompanyStateMachine.
Lines 112-123 explicitly catch
DoesNotExistand log an error whenCompanyStateMachineis missing, while this change silently returnsNone. Although the guard on line 257 prevents a crash, the lack of logging here may hide misconfigurations where a state machine is expected but missing.📝 Proposed fix to add logging when state machine is missing
state_machine = CompanyStateMachine.objects.filter( company_bot=self.company_bot, step=chat_session.current_step ).first() + +if not state_machine: + logger.warning( + f"CompanyStateMachine not found for bot_id={self.company_bot.id}, " + f"step={chat_session.current_step} in translate_message. " + f"Defaulting to translation without state-specific text_conversion_type." + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@chatbot/consumers/async_consumer.py` around lines 253 - 255, The change uses CompanyStateMachine.objects.filter(...).first() which can return None without logging; add an error log when state_machine is None (same place where DoesNotExist was previously handled) that includes identifying info like self.company_bot (or company_bot id) and chat_session.current_step, then return None as before; locate the state_machine assignment and insert a logger/process_logger.error call (matching the existing logging convention in this file) to report the missing CompanyStateMachine.chatbot/celery_tasks/title_tasks.py (1)
77-77: 💤 Low valueConsider unpacking instead of list concatenation.
Ruff suggests using unpacking syntax for better readability and performance.
♻️ Proposed refactor
response = call_llm_gateway( - messages=[system_msg] + list(messages), + messages=[system_msg, *messages], provider=company_bot.provider, model=company_bot.llm_model, params=build_gateway_params(company_bot),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@chatbot/celery_tasks/title_tasks.py` at line 77, Replace the list concatenation "messages=[system_msg] + list(messages)" with the clearer unpacking form "messages=[system_msg, *messages]" in title_tasks.py; this keeps the same semantics but is more readable and avoids the extra list() allocation—ensure "messages" is an iterable (if it's a generator that must be consumed elsewhere, convert it before unpacking) and update the call site where system_msg and messages are combined in the task function.chatbot/views/api_views.py (1)
215-222: ⚡ Quick winRedundant
bool()calls and alias.Lines 215-217 and 219-221 wrap
.get('...', False)inbool(), but theget()method already returns the booleanFalseas the default, making the outerbool()redundant. Additionally, line 222 createsis_profile_completeas an alias foris_onboarding_completedwithout adding semantic value.♻️ Simplify boolean extraction and remove alias
- is_tnc_accepted = bool( - profile.other_params and profile.other_params.get('is_tnc_accepted', False) - ) - - is_onboarding_completed = bool( - profile.other_params and profile.other_params.get('is_onboarding_completed', False) - ) - is_profile_complete = is_onboarding_completed + is_tnc_accepted = (profile.other_params or {}).get('is_tnc_accepted', False) + is_profile_complete = (profile.other_params or {}).get('is_onboarding_completed', False)Then update line 236 to use
is_profile_completedirectly without the intermediateis_onboarding_completedvariable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@chatbot/views/api_views.py` around lines 215 - 222, Remove the redundant bool() wrappers around profile.other_params.get(...) by assigning is_tnc_accepted = profile.other_params and profile.other_params.get('is_tnc_accepted', False) and is_profile_complete = profile.other_params and profile.other_params.get('is_onboarding_completed', False); delete the intermediate is_onboarding_completed variable and then update any later usages (e.g., the reference at the previous line 236) to use is_profile_complete directly instead of is_onboarding_completed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@chatbot/celery_tasks/title_tasks.py`:
- Around line 114-123: The translation can fail because text_translate_provider
is called with voice_provider possibly None but without the company_bot
fallback; update the call in title_tasks.py so it passes company_bot (the same
variable used to query Voice.objects.filter) into text_translate_provider along
with voice_provider, message_body=output_title, target_language=language and
source_language='en' and keep the existing check of translated.get('status') ==
200 to assign output_title; this ensures text_translate_provider can resolve a
fallback provider when Voice.objects.filter(...) returns None.
In `@chatbot/models/chat_models.py`:
- Around line 28-31: The save_title method signature was changed and now breaks
callers; restore backward compatibility by changing save_title to accept
optional parameters: def save_title(self, title=None, language=None) and keep
the existing behavior of only setting and saving self.title when a non-empty
title is provided (leave language unused for now). Update the body to check if
title is truthy and self.title is falsy before assigning and calling
self.save(update_fields=['title']) so calls like save_title(),
save_title(language=...), and save_title(title=...) all work without raising
TypeError; keep the method name save_title to match callers in
chatbot/utils/story_utils/story_utils.py,
chatbot/scripts/meghaPTM/create_story_script.py,
chatbot/consumers/base_consumer.py and tests.
In `@chatbot/models/geo_models.py`:
- Around line 28-29: The __str__ in the model returns str(self.profile.email)
which yields "None" when email is None; update the __str__ method (in the class
defining __str__) to check for a real email and provide a user-friendly fallback
(e.g., return self.profile.first_name or self.profile.email or "<no email>" or
another meaningful identifier like self.profile.username or id) so that if
self.profile.email is None it does not display the literal "None".
In `@chatbot/utils/media_preview/media_creation.py`:
- Around line 272-273: Docstring for _get_flow_title incorrectly mentions
constants_json['title'] while the function uses constants_json['doc_title'];
update the docstring to reference constants_json['doc_title'] (or change the key
access to 'title' if intended) so the description matches the implementation,
and ensure the function name _get_flow_title and the constants_json key name are
consistent.
In `@chatbot/views/api_views.py`:
- Around line 204-212: Both endpoints (get_profile_view, accept_tnc_view) share
non-deterministic fallback logic using Company.objects.order_by('id').first()
when company_slug is missing; extract that into a helper (e.g.,
resolve_company(company_slug, require_slug_for_email=False)) and replace the
duplicated code. Implement resolve_company to: 1) return
Company.objects.get(slug=company_slug) if company_slug provided; 2) if not
provided, check an env var DEFAULT_COMPANY_SLUG and return that company if set;
3) otherwise, when require_slug_for_email is True (used by endpoints that lookup
by email like get_profile_view), raise/return a 400 response forcing the caller
to supply company_slug; else return a clear 404/error instead of silently using
Company.objects.order_by('id').first(). Update get_profile_view and
accept_tnc_view to call resolve_company and handle the helper's 400/404
semantics accordingly.
In `@shikshalokam_mohini/settings.py`:
- Around line 28-33: The repo still contains legacy
"shikshalokam-mohini-service" references that must be renamed to
"saathi-backend" to avoid broken deploys; update occurrences referenced by
load_secrets (paths_to_try in load_secrets) and then update docker-compose.yml
(image names and log volume paths), README.md and docs/setup/developer_setup.md
(local project paths/examples), and manage.spec / celery_worker.spec (replace
/home/ubuntu/shikshalokam-mohini-service/... with
/home/ubuntu/saathi-backend/...) so all image names, volume mounts, and
documented paths consistently use saathi-backend. Ensure exact string
replacements preserve surrounding syntax (YAML, spec files, markdown) and run a
quick grep to verify no remaining "shikshalokam-mohini-service" tokens.
---
Nitpick comments:
In `@chatbot/celery_tasks/title_tasks.py`:
- Line 77: Replace the list concatenation "messages=[system_msg] +
list(messages)" with the clearer unpacking form "messages=[system_msg,
*messages]" in title_tasks.py; this keeps the same semantics but is more
readable and avoids the extra list() allocation—ensure "messages" is an iterable
(if it's a generator that must be consumed elsewhere, convert it before
unpacking) and update the call site where system_msg and messages are combined
in the task function.
In `@chatbot/consumers/async_consumer.py`:
- Around line 253-255: The change uses
CompanyStateMachine.objects.filter(...).first() which can return None without
logging; add an error log when state_machine is None (same place where
DoesNotExist was previously handled) that includes identifying info like
self.company_bot (or company_bot id) and chat_session.current_step, then return
None as before; locate the state_machine assignment and insert a
logger/process_logger.error call (matching the existing logging convention in
this file) to report the missing CompanyStateMachine.
In `@chatbot/views/api_views.py`:
- Around line 215-222: Remove the redundant bool() wrappers around
profile.other_params.get(...) by assigning is_tnc_accepted =
profile.other_params and profile.other_params.get('is_tnc_accepted', False) and
is_profile_complete = profile.other_params and
profile.other_params.get('is_onboarding_completed', False); delete the
intermediate is_onboarding_completed variable and then update any later usages
(e.g., the reference at the previous line 236) to use is_profile_complete
directly instead of is_onboarding_completed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f39e7bbb-fb3f-4a3e-ac8d-95c364206d61
📒 Files selected for processing (16)
chatbot/admin/company_admin.pychatbot/celery_tasks/handle_message.pychatbot/celery_tasks/title_tasks.pychatbot/consumers/async_base_consumer.pychatbot/consumers/async_consumer.pychatbot/migrations/0085_flow_title_bot_historicalflow_title_bot_and_more.pychatbot/models/chat_models.pychatbot/models/company_models.pychatbot/models/geo_models.pychatbot/services/response_handlers/common_handler.pychatbot/urls.pychatbot/utils/elevate/profile_utils.pychatbot/utils/media_preview/media_creation.pychatbot/views/api_views.pyshikshalokam_mohini/celery_config.pyshikshalokam_mohini/settings.py
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
reviewed |
2491f60
into
ELEVATE-Project:release-1.0.0
Summary by CodeRabbit
New Features
Improvements