Yashan-New#38
Conversation
WalkthroughUpdated core configuration model: adjusted string literal style for env_file_encoding, added new PUBLIC_URL (str) and TEST_MODE (bool, default False) fields to Settings, and applied non-functional formatting changes. Changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
frontend/src/store/campaignStore.js (1)
22-33: Align method names, add alias, and gate debug logsThe store’s
fetchCampaignDetailswas renamed without updating all call sites, and unguardedconsole.logcalls remain. Please address the following:• In
frontend/src/store/campaignStore.js
– Wrap bothconsole.logcalls inif (process.env.NODE_ENV !== 'production') { … }.
– Ensure there’s no stray character at EOF (runtail -n5 | sed -n 'l'to confirm).
– Add a temporary alias so existing pages don’t break:
diff fetchCampaignDetails: async (id) => { if (process.env.NODE_ENV !== 'production') { console.log('Store: fetchCampaignDetails called with ID:', id); } … }, +// alias for backward compatibility; remove once all call sites updated +fetchCampaignById: async (id) => get().fetchCampaignDetails(id),• In
frontend/src/api/campaignApi.js
– Confirm the API method matches what the store calls. Either renamegetCampaignById→getCampaignDetailsor vice versa, and updatecampaignStore.jsaccordingly.• In all pages (e.g.
CampaignDetailPage.jsx)
– ReplacefetchCampaignById(...)withfetchCampaignDetails(...)once the alias is removed.These changes will prevent runtime errors from mismatched method names and reduce console noise in production.
🧹 Nitpick comments (13)
frontend/src/components/CompaignDetail.jsx (2)
11-18: Add empty state for contacts.Show a helpful message when there are no contacts instead of rendering an empty list.
Apply this diff:
- <h4>Contacts:</h4> - <ul> - {campaign.contacts?.map((contact) => ( - <li key={contact.id}> - {contact.name} - {contact.callStatus} - </li> - ))} - </ul> + <h4>Contacts:</h4> + <ul> + {Array.isArray(campaign.contacts) && campaign.contacts.length > 0 ? ( + campaign.contacts.map((contact) => ( + <li key={contact.id}> + {contact.name} - {contact.callStatus} + </li> + )) + ) : ( + <li>No contacts yet.</li> + )} + </ul>
2-2: Optional: Add PropTypes for runtime type-safety.Keeps the component robust and self-documented.
-import React from 'react'; +import React from 'react'; +import PropTypes from 'prop-types'; @@ }; +CampaignDetail.propTypes = { + campaign: PropTypes.shape({ + name: PropTypes.string, + status: PropTypes.string, + contacts: PropTypes.arrayOf( + PropTypes.shape({ + id: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired, + name: PropTypes.string, + callStatus: PropTypes.string, + }) + ), + }), +}; + export default CampaignDetail;Also applies to: 21-23
frontend/src/pages/CampaignsPage.jsx (3)
17-22: Remove or gate debug logs.Console logging campaigns on every change can be noisy in production. Guard with NODE_ENV or remove.
- useEffect(() => { - if (campaigns.length > 0) { - console.log('Available campaigns:', campaigns.map(c => ({ id: c.id, name: c.name }))); - } - }, [campaigns]); + useEffect(() => { + if (process.env.NODE_ENV !== 'production' && campaigns.length > 0) { + console.log('Available campaigns:', campaigns.map(c => ({ id: c.id, name: c.name }))); + } + }, [campaigns]);
37-43: Use event.currentTarget for backdrop detection (class name–agnostic).This avoids coupling to CSS class and is more robust.
- const handleBackdropClick = useCallback((e) => { - if (e.target.classList.contains('modal-backdrop')) { - setIsModalOpen(false); - } - }, []); + const handleBackdropClick = useCallback((e) => { + if (e.target === e.currentTarget) { + setIsModalOpen(false); + } + }, []);
59-64: Improve modal accessibility with dialog semantics.Add role="dialog" and aria-modal on the modal container; provide a label.
- <div className="modal-backdrop" onClick={handleBackdropClick}> - <div className="modal-content card"> + <div className="modal-backdrop" onClick={handleBackdropClick}> + <div + className="modal-content card" + role="dialog" + aria-modal="true" + aria-label="Create New Campaign" + > <CampaignForm onFormSubmit={() => setIsModalOpen(false)} /> <Button className="modal-close-button" onClick={() => setIsModalOpen(false)}>Close</Button> </div> </div>frontend/src/App.jsx (1)
4-4: Clean up commented-out code before merge.Avoid shipping commented imports and blocks; keeps codebase clean.
Also applies to: 12-14, 20-21
frontend/src/components/CampaignForm.jsx (2)
59-66: Disable the agent selector when no agents are available.Prevents confusing required-validation states when the list is empty.
<select id="agent_id" name="agent_id" value={agentId} onChange={(e) => setAgentId(e.target.value)} required - className="input" + className="input" + disabled={agents.length === 0} > <option value="" disabled>-- Choose an agent --</option> {agents.length === 0 ? ( <option disabled>No agents available</option> ) : ( agents.map(agent => ( <option key={agent.id} value={agent.id}> {agent.name} (ID: {agent.id}) </option> )) )}Also applies to: 68-76
80-82: Disable submit when no agents are available.Improves UX and avoids futile submissions.
- <Button type="submit" disabled={submitting}> - {submitting ? 'Creating...' : 'Create Campaign'} + <Button type="submit" disabled={submitting || agents.length === 0}> + {submitting ? 'Creating...' : (agents.length === 0 ? 'No Agents' : 'Create Campaign')} </Button>frontend/src/store/campaignStore.js (3)
35-44: Global loading flag used for create; consider per-action flags.Toggling the shared loading state during creation can cause the main page to show a global spinner behind the modal. Consider per-action flags (e.g., creating, deleting, starting, stopping) to avoid UI flicker.
57-71: Start campaign flow looks correct; minor consistency suggestion.LGTM on flow and conditional detail refresh. Consider normalizing error messages via a helper for consistency across actions.
73-86: Stop campaign flow is sound; align error message extraction.Same as start; consider consistent error extraction (e.g., error.response?.data?.detail || error.message).
- } catch (error) { - console.error('Error stopping campaign:', error); - set({ error: error.message, loading: false }); + } catch (error) { + console.error('Error stopping campaign:', error); + const msg = error?.response?.data?.detail || error?.message; + set({ error: msg, loading: false }); }frontend/src/components/CampaignList.jsx (1)
75-80: Prefer CSS classes over inline style for the “View” buttonInline styles make theming harder and bypass CSS minification/purging. Move the colour declaration into
action-linkor a dedicated utility class.frontend/src/services/campaignApi.js (1)
4-6: Stripconsole.login production buildsThe global
console.log('API Base URL:', …)leaks implementation details and clutters logs. Gate all debug logging behindprocess.env.NODE_ENV !== 'production'or use a proper logger.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/src/App.jsx(1 hunks)frontend/src/components/CampaignForm.jsx(2 hunks)frontend/src/components/CampaignList.jsx(4 hunks)frontend/src/components/CompaignDetail.jsx(1 hunks)frontend/src/pages/CampaignsPage.jsx(3 hunks)frontend/src/services/campaignApi.js(2 hunks)frontend/src/store/campaignStore.js(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/src/pages/CampaignsPage.jsx (2)
frontend/src/components/common/Button.jsx (1)
Button(4-10)frontend/src/components/CampaignList.jsx (1)
CampaignList(7-120)
frontend/src/store/campaignStore.js (2)
frontend/src/components/CampaignList.jsx (1)
useCampaignStore(8-12)frontend/src/pages/CampaignDetailPage.jsx (4)
useCampaignStore(11-11)campaignId(31-40)stopCampaign(133-143)loadStatus(30-53)
🔇 Additional comments (3)
frontend/src/pages/CampaignsPage.jsx (1)
24-35: ESC-to-close modal: implementation looks good.frontend/src/App.jsx (1)
15-18: Nice a11y improvement with role="main".frontend/src/components/CampaignList.jsx (1)
81-94: Guard against case-mismatch in status comparisons
disabled={campaign.status === 'running'}and the equivalent for stopped assume lower-case API values, while other code treats status as mixed-case (e.g..toLowerCase()for CSS). If the backend returns"RUNNING"/"STOPPED", the buttons will never disable.Either normalise with
.toLowerCase()or compare against enumerated constants shared with the API model.
| // frontend/src/components/CampaignDetail.jsx | ||
| import React from 'react'; | ||
|
|
||
| const CampaignDetail = ({ campaign }) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: filename typo breaks imports (CompaignDetail vs CampaignDetail).
The file path/name is misspelled (“CompaignDetail.jsx”) while the header comment shows “CampaignDetail.jsx”. This will cause import mismatches.
Action:
- Rename the file to frontend/src/components/CampaignDetail.jsx and update all imports.
🏁 Script executed:
#!/bin/bash
# Locate actual file and any imports referencing either spelling
fd -a 'CompaignDetail.jsx' || true
fd -a 'CampaignDetail.jsx' || true
rg -n "from ['\"][^'\"]*CompaignDetail|from ['\"][^'\"]*CampaignDetail" -g "!node_modules"Length of output: 329
🏁 Script executed:
#!/bin/bash
# Locate the CampaignDetailPage.jsx file
fd -a CampaignDetailPage.jsx
# Show all import statements in the page
echo "=== Imports in frontend/src/pages/CampaignDetailPage.jsx ==="
grep -n '^import' frontend/src/pages/CampaignDetailPage.jsx || true
# Check for any references to the component filename (typo or correct)
echo "=== References to ‘CompaignDetail’ or ‘CampaignDetail’ ==="
grep -n 'CompaignDetail\|CampaignDetail' frontend/src/pages/CampaignDetailPage.jsx || trueLength of output: 1033
Rename component file for consistency
The component file is currently named CompaignDetail.jsx (typo) while its header comment and component name use CampaignDetail. Although there are no existing imports of this component (it’s unused right now), mismatched filenames will cause confusion and future import errors. Please:
- Rename
frontend/src/components/CompaignDetail.jsx→frontend/src/components/CampaignDetail.jsx - Update the top-of-file comment to
// frontend/src/components/CampaignDetail.jsx - When importing elsewhere, use the corrected path and name (
CampaignDetail)
No other imports need to be updated at this time.
🤖 Prompt for AI Agents
In frontend/src/components/CompaignDetail.jsx at lines 1 to 4, rename the file
from CompaignDetail.jsx to CampaignDetail.jsx to fix the typo. Update the
top-of-file comment to reflect the new filename as //
frontend/src/components/CampaignDetail.jsx. Ensure that future imports use the
corrected filename and component name CampaignDetail to avoid confusion and
import errors.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file with correct spelling exists
echo "=== Checking for CampaignDetail.jsx (correct spelling) ==="
fd -t f "CampaignDetail.jsx" --full-path
# Check if the file with typo still exists
echo -e "\n=== Checking for CompaignDetail.jsx (typo) ==="
fd -t f "CompaignDetail.jsx" --full-path
# Check the actual content of the correctly named file if it exists
if [ -f "frontend/src/components/CampaignDetail.jsx" ]; then
echo -e "\n=== Content of CampaignDetail.jsx (first 5 lines) ==="
head -5 frontend/src/components/CampaignDetail.jsx
fi
# Check for any imports of this component
echo -e "\n=== Checking for imports of CampaignDetail component ==="
rg "import.*CampaignDetail" --type js --type jsx -g "!node_modules"
Length of output: 966
🏁 Script executed:
#!/bin/bash
# Check for any imports of CampaignDetail component without type flag
echo "=== Checking for imports of CampaignDetail component ==="
rg "import.*CampaignDetail" -g "*.js" -g "*.jsx" -g "!node_modules"
# Also check if this component is used anywhere
echo -e "\n=== Checking for usage of CampaignDetail component ==="
rg "<CampaignDetail" -g "*.js" -g "*.jsx" -g "!node_modules"
Length of output: 630
@YashanPreetKaur Perfect! The file has been successfully renamed from CompaignDetail.jsx to CampaignDetail.jsx. The header comment and component name are all correctly aligned now.
Since the component isn't currently imported anywhere in the codebase, no additional import updates are needed. The file is ready for future use with the correct naming convention.
…e, API header, syntax cleanup
|
@Hiteshydv001 Fixed as per suggestion — added trim, improved error handling, and renamed file |
|
@YashanPreetKaur Would you provide me some demo |
|
@213sanjana What kind of demo you want and in how much time? |
|
Working demo u can give a video |
|
yes sure! can i provide you within 1-2 days |
Sure how about tomorrow so that we could go ahead with the work |
|
why campaigns are showing like this now 
|
Please properly atttach ss of what you have changed |
|
@YashanPreetKaur These are the original repo elements |
|
Give me some time, I'll update asap |
|
Any updates |
|
I tried running it on my system, but my Docker setup isn’t accessing the code properly, so I’m unable to execute it right now. Could you please let me know the exact changes I need to make? Once I have those, I’ll update the code accordingly. |
|
Please add new campaigns into your list, It'll show the list |
|
You are working on issue #19 in which most of the things are implemented already |
|
i think it will be good if n add additional functuionality to the elements mentioned in the issue |
|
after reviewing code i find out most of the code looks same as we have earlier some of the things got deleted |
|
if you can enhance the related things to campaings and all , it will be good |
Hiteshydv001
added
GSSoc'25
Level 3
|
I’ve made the required changes as mentioned in the issue |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/core/config.py (3)
25-26: Type PUBLIC_URL as a validated URL and drop inline note/emoji.
Using Pydantic’s URL types provides early validation and clearer errors if misconfigured. Also, remove the “✅ added earlier” inline note—these editorial comments add noise in code.Apply this diff:
- PUBLIC_URL: str # ✅ added earlier + PUBLIC_URL: HttpUrlAdd this import near the top:
from pydantic import HttpUrlNote: If existing code treats
PUBLIC_URLas a plain string, wrap where needed:str(settings.PUBLIC_URL).
27-28: Prefer Field metadata and remove inline note; keep default behavior identical.
This documents intent, keeps default False, and removes the editorial comment.Apply this diff:
- # Misc - TEST_MODE: bool = False # ✅ new line + # Misc + TEST_MODE: bool = Field(default=False, description="Enable test stubs/mocks and non-destructive behavior where supported")Add this import near the top:
from pydantic import Field
30-31: Use a lazy-cached factory for Settings to defer instantiation and improve testabilityInstantiating
Settings()at import time can cause early crashes if a required environment variable is missing and makes it harder to override in tests. A small refactor using@lru_cachedefers creation until first use while preserving performance.Proposed change in
backend/src/core/config.py:-from core.config import Settings - -# module-level singleton (current) -settings = Settings() +from functools import lru_cache + +@lru_cache(maxsize=1) +def get_settings() -> Settings: + return Settings() + +# (optional) thin alias to preserve existing imports; +# drawback: still instantiates at import time +settings = get_settings()Affected files—update imports/usages to call
get_settings()(or keep the alias above):
- backend/src/services/tts_service.py
- backend/src/services/stt_service.py
- backend/src/services/telephony_service.py
- backend/src/services/llm_service.py
- backend/src/main.py
Environment-variable templates need updating—neither
.env.examplenor any backend.env.*currently definePUBLIC_URLorTEST_MODE. Add to your templates (and CI/CD/deploy configs) something like:PUBLIC_URL=<your backend URL> TEST_MODE=falseEnsure all deployment/testing environments are configured with these keys before merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/src/core/config.py(2 hunks)
🔇 Additional comments (2)
backend/src/core/config.py (2)
5-5: Quote-style change is a no-op; model_config usage looks correct.
Switching to double quotes doesn’t affect behavior.SettingsConfigDictonmodel_configis idiomatic for Pydantic v2.
21-21: No action needed for whitespace-only change.
This looks like a formatting-only edit.
|
@YashanPreetKaur these all are already implemented |
|
I have fixed the issue as discussed. Please review properly, and if everything looks good, kindly merge it as my contribution. |
|
what changes have you implemented describe precisely |
|
only one file is chnaged with no changes at all |
3 participants
Updated campaign management frontend with improved UI, safer deletion via confirmation modal, better form validation, and cleaner code structure across CampaignForm, CampaignList, CampaignsPage, and App components.
Summary by CodeRabbit