feat: add campaign lead unenroll action#1
Open
saurabhhhcodes wants to merge 94 commits into
Open
Conversation
…ger-zone-24 Reviewed the implementation. The danger zone actions are implemented correctly with appropriate confirmation safeguards: * Delete All Leads action includes confirmation before destructive execution. * Delete Organization flow correctly validates the organization name before deletion. * Backend actions and related tests are included. * Frontend integration and UX behavior are properly connected. The implementation looks clean and aligned with the intended functionality. Approved for merge.
…anding-23 fix: unify LeadOrbit backend branding
test: add tenant isolation tests for DashboardAnalyticsView (fixes Kuldeeep18#26)
…ini-model-upgrade feat: upgrade gemini engine to 2.0-flash and implement multi-tenant k…
feat: add dark/light mode toggle with theme persistence
feat: add unsubscribe page and one-click unsubscribe support
…-shortcuts-67 feat: implement global keyboard shortcuts and helper navigation menu …
…kend-envvar Fix Kuldeeep18#70: Use CELERY_RESULT_BACKEND env var instead of CELERY_BROKER_URL
feature: add password visibility toggle for login and register pages
Fix Sidebar Active Link & Remove Duplicate CSS in campaigns.html
…ng-guide docs: add CONTRIBUTING.md with setup guide and contribution rules
feat: added custom 404 Not Found Page
…mb-navigation feat: add breadcrumb navigation to all authenticated pages
Add CSV custom variable support
…omain-list Add global unsubscribe domain blocklist
…k-logging Log email webhook processing errors
Add SEO and social media meta tags (issue Kuldeeep18#39)
Add Back to Home button to login and register pages
Improve readme by adding Contributors section with contrib.rocks badge
…confirm-modals feat: replace confirm() and prompt() with Bootstrap modals
…y-improvements Fix Kuldeeep18#64: Improve accessibility (a11y) across frontend pages
…conduct docs: add CODE_OF_CONDUCT.md with reference in README
…ttings-modal feat: implement campaign settings modal
…s-cta feat: add import leads CTA to campaign builder empty state
LO-014 [Medium]: Add Email Template Library
…eme-toggle-129 Fix settings dark mode toggle timing
…ggle-126 fix: improve password visibility toggle styling
📝 WalkthroughWalkthroughThis PR introduces substantial feature additions and UX improvements across the full platform. The backend gains email templates, lead blocking by domain, custom variables on leads, unsubscribe token infrastructure, and campaign lifecycle management. The frontend receives a complete theme system with dark mode support, improved navigation with breadcrumbs and hamburger menus, skeleton loading states, and major updates to the campaign builder including template management and campaign settings. All changes are supported by comprehensive test coverage and updated project documentation. ChangesBackend Infrastructure & Data Models
Backend API Layer
Email and Unsubscribe Infrastructure
Campaign Processing Logic
CSV and Data Handling
Backend Tests
Frontend Theme and Shell
Frontend Pages
Campaign Builder Major Updates
Frontend API Client
Settings Page
Documentation and Project Setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/leads.html (1)
230-249:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPotential XSS via unescaped
lead.emailin HTML template.
lead.emailis interpolated directly into the HTML template without escaping (line 234 in the<span>and line 237 indata-email). A malicious email address like"><script>alert(1)</script>could execute arbitrary JavaScript.Proposed fix
Add an escape helper (similar to
escapeHtmlin campaigns.html) and use it:+ const escapeHtml = (value) => String(value ?? '') + .replaceAll('&', '&') + .replaceAll('<', '<') + .replaceAll('>', '>') + .replaceAll('"', '"') + .replaceAll("'", '&`#39`;'); + function renderLeadRows(leads) { // ... tbody.innerHTML = leads.map(lead => ` <tr> - <td class="fw-semibold">${lead.first_name || ''} ${lead.last_name || ''}</td> + <td class="fw-semibold">${escapeHtml(lead.first_name)} ${escapeHtml(lead.last_name)}</td> <td> - <span>${lead.email}</span> + <span>${escapeHtml(lead.email)}</span> <button class="btn btn-sm btn-outline-secondary copy-email-btn" - data-email="${lead.email}" + data-email="${escapeHtml(lead.email)}" aria-label="Copy email address" >🤖 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 `@frontend/leads.html` around lines 230 - 249, The template that sets tbody.innerHTML uses unescaped lead data (see the leads.map(...) template) which allows XSS via lead.email and other fields; add or reuse an escapeHtml(string) helper (as in campaigns.html) and apply it to all interpolated user-controlled values (e.g., lead.email, lead.first_name, lead.last_name, lead.phone, lead.company, lead.score, and lead.global_unsubscribe output) before inserting into the template and also use the escaped value for the data-email attribute to ensure no HTML/JS can be injected.frontend/dashboard.html (1)
191-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkeletons remain visible when the fetch fails.
If
fetchWithAuth('/analytics/dashboard/')throws or returns a non-OK response, the skeleton loaders (.skeleton-kpi,#chart-skeleton,#activity-skeletons) stay visible indefinitely, leaving users with a perpetual loading state instead of an error message or the empty-state UI.Proposed fix
} catch (e) { console.error('Dashboard fetch error', e); + // Hide skeletons and show empty/fallback state on error + document.querySelectorAll('.skeleton-kpi').forEach(el => el.style.display = 'none'); + document.querySelectorAll('.kpi-value-text').forEach(el => el.style.display = 'block'); + const chartSkeleton = document.getElementById('chart-skeleton'); + if (chartSkeleton) chartSkeleton.style.display = 'none'; + const chartContent = document.getElementById('chart-content'); + if (chartContent) chartContent.style.display = 'flex'; + const actSkeletons = document.getElementById('activity-skeletons'); + if (actSkeletons) actSkeletons.style.display = 'none'; + const actList = document.getElementById('activity-list'); + if (actList) actList.style.display = 'block'; }🤖 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 `@frontend/dashboard.html` around lines 191 - 216, The fetch failure path currently leaves skeleton loaders visible; update the logic around fetchWithAuth('/analytics/dashboard/') so that both non-OK responses and exceptions hide the same skeleton elements ('.skeleton-kpi', '`#chart-skeleton`', '`#activity-skeletons`') and reveal an error/empty-state UI. Specifically, inside the else branch for if (!res.ok) and inside the catch(e) block, call the same DOM updates you do on success to hide skeletons and show fallback elements (e.g., hide '.skeleton-kpi' and '`#chart-skeleton`', hide '`#activity-skeletons`', show '.kpi-value-text' or '`#activity-list`' as appropriate) and display an error element (e.g., '`#dashboard-error`' or create one) with a user-friendly message; make the change around the fetchWithAuth usage and the DOM selectors used in the success path to ensure consistent UI in all paths.
🟡 Minor comments (15)
CODE_OF_CONDUCT.md-50-55 (1)
50-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the duplicated heading and render the report link properly.
Reporting Guidelinesappears twice, and the discussions URL is pasted as plain text. Please keep one heading and make the link clickable Markdown.🤖 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 `@CODE_OF_CONDUCT.md` around lines 50 - 55, Remove the duplicated "Reporting Guidelines" heading so only one remains and update the plain URL to a proper Markdown link; locate the "Reporting Guidelines" heading in CODE_OF_CONDUCT.md and replace the line containing the raw URL "(https://github.com/Kuldeeep18/LeadOrbit/discussions)" with a clickable Markdown link such as "[project discussions](https://github.com/Kuldeeep18/LeadOrbit/discussions)" (or similar descriptive text) and ensure the contact mention "**`@Kuldeeep18`**" and surrounding sentence remain intact and grammatically correct.README.md-23-30 (1)
23-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the duplicated README block and replace the placeholder section.
The file appears to contain two copies of the README, and the first copy still leaves
## Contributingas a stub. Keep one canonical version and point contributors to the real guide.Also applies to: 236-467
🤖 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 `@README.md` around lines 23 - 30, Remove the duplicated README content by keeping a single canonical copy (retain the headings "## Code of Conduct" and "## Contributing") and delete the redundant block; replace the placeholder "## Contributing" stub with a real pointer to the contribution guide (for example: reference CONTRIBUTING.md or the project's contribution URL and brief steps for PRs/issues), and ensure the removed duplicate content (the other copy later in the file) is fully deleted so only one complete README remains.frontend/register.html-6-6 (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate title tag.
Two
<title>tags are present (lines 6 and 22). Keep only one to avoid redundancy.🔧 Proposed fix
- <!-- SEO Meta Tags --> - <title>LeadOrbit - Register</title> <meta name="description" content="LeadOrbit - Create your account. Start managing leads and campaigns.">Also applies to: 22-22
🤖 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 `@frontend/register.html` at line 6, There are two duplicate <title> elements in register.html; remove one so only a single <title> remains in the document head (keep the intended text "LeadOrbit - Register"), ensuring any metadata or script references remain unchanged—look for the duplicate <title> tags and delete the redundant one.frontend/login.html-6-6 (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate title tag.
Two
<title>tags are present (lines 6 and 21). Browsers will use the last one, making the first redundant. Keep only one title tag.🔧 Proposed fix
- <!-- SEO Meta Tags --> - <title>LeadOrbit - Login</title> <meta name="description" content="LeadOrbit - Login to your account. Manage leads, campaigns, and outreach.">Also applies to: 21-21
🤖 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 `@frontend/login.html` at line 6, Remove the duplicate <title> tag so only one remains in the document: locate the two occurrences of the <title> element in the login HTML and delete the redundant one (keep the intended page title "LeadOrbit - Login" in a single <title> element), ensuring the head contains only a single <title> tag.frontend/unsubscribe.html-6-6 (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate title tag.
Two
<title>tags are present (lines 6 and 22). Keep only one to avoid redundancy.🔧 Proposed fix
- <!-- SEO Meta Tags --> - <title>LeadOrbit - Unsubscribe</title> <meta name="description" content="LeadOrbit - Unsubscribe from email communications. Manage your notification preferences.">Also applies to: 22-22
🤖 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 `@frontend/unsubscribe.html` at line 6, Remove the duplicate <title> element in the document head so only a single <title> tag remains; locate the two <title> tags (the one shown in the diff and the other later in the head) and delete one of them, keeping the correct/desired page title text within the remaining <title> element.frontend/analytics.html-6-6 (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
<title>tags in<head>.Same issue as other pages - lines 6 and 21 both define a
<title>element.Proposed fix
<head> <!-- SEO Meta Tags --> - <title>LeadOrbit - Analytics</title> <meta name="description" content="LeadOrbit - Analytics. Track campaign performance, open rates, click rates, and engagement metrics.">Also applies to: 21-21
🤖 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 `@frontend/analytics.html` at line 6, The head of frontend/analytics.html contains duplicate <title> elements; remove the redundant <title> so there is exactly one <title> in the <head> (keep the intended text "LeadOrbit - Analytics" or consolidate any differing text into that single tag) and ensure no other <title> tags remain in the same document head (look for the <title> tags in the file to locate and fix).frontend/dashboard.html-6-6 (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
<title>tags in<head>.Lines 6 and 21 both define a
<title>element. Only one<title>tag is valid per HTML document; having two may cause browsers to use just one (typically the last) and can confuse SEO crawlers.Remove the duplicate:
Proposed fix
<head> <!-- SEO Meta Tags --> - <title>LeadOrbit - Dashboard</title> <meta name="description" content="LeadOrbit - Dashboard. View campaign performance, lead analytics, and key metrics.">Also applies to: 21-21
🤖 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 `@frontend/dashboard.html` at line 6, There are two <title> elements inside the document head; remove the duplicate so only one <title> element remains (keep the intended "LeadOrbit - Dashboard" title), by deleting the extra <title> tag found in the <head> section and ensuring only a single <title> element exists within the <head>.frontend/campaigns.html-6-6 (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
<title>tags in<head>.Same issue as other pages - lines 6 and 21 both define a
<title>element.Proposed fix
<head> <!-- SEO Meta Tags --> - <title>LeadOrbit - Campaigns</title> <meta name="description" content="LeadOrbit - Campaigns. Create, launch, and manage multi-channel outreach campaigns.">Also applies to: 21-21
🤖 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 `@frontend/campaigns.html` at line 6, The head contains duplicate <title> elements (both set to "LeadOrbit - Campaigns"); remove the redundant <title> so there is exactly one <title> element in the <head> (keep the correct "LeadOrbit - Campaigns" title) and ensure any other metadata remains intact; also scan other pages for the same duplicate <title> pattern and fix them similarly.frontend/settings.html-6-6 (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
<title>tags in<head>.Same issue as other pages - lines 6 and 21 both define a
<title>element.Proposed fix
<head> <!-- SEO Meta Tags --> - <title>LeadOrbit - Settings</title> <meta name="description" content="LeadOrbit - Settings. Configure your account, integrations, API keys, and preferences.">Also applies to: 21-21
🤖 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 `@frontend/settings.html` at line 6, There are two <title> elements in the <head> for this page causing duplicate document titles; remove the redundant <title> so only a single <title> "LeadOrbit - Settings" remains within the <head> section (ensure any metadata or templating that generates the second <title> is removed or consolidated so only one <title> tag is present).frontend/leads.html-6-6 (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
<title>tags in<head>.Same issue as other pages - lines 6 and 21 both define a
<title>element.Proposed fix
<head> <!-- SEO Meta Tags --> - <title>LeadOrbit - Lead Management</title> <meta name="description" content="LeadOrbit - Lead Management. Import, manage, and track your leads and engagement.">Also applies to: 21-21
🤖 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 `@frontend/leads.html` at line 6, The head of leads.html contains duplicate <title> elements; remove the redundant <title> so there is only a single <title> ("LeadOrbit - Lead Management") in the document head (locate both <title> occurrences and delete the duplicate), ensuring the retained title is the intended one and that no other head metadata is accidentally removed.frontend/leads.html-186-198 (1)
186-198:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
copyToClipboardsilently fails if clipboard API is unavailable.
navigator.clipboard.writeTextcan fail (e.g., non-HTTPS context, permission denied). The.then()handler has no.catch(), so failures are silently ignored and the user gets no feedback.Proposed fix
function copyToClipboard(text, button) { navigator.clipboard.writeText(text).then(() => { const icon = button.querySelector('i'); icon.classList.remove('bi-clipboard'); icon.classList.add('bi-check'); setTimeout(() => { icon.classList.remove('bi-check'); icon.classList.add('bi-clipboard'); }, 2000); + }).catch(() => { + console.warn('Clipboard copy failed'); }); }🤖 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 `@frontend/leads.html` around lines 186 - 198, The copyToClipboard function currently calls navigator.clipboard.writeText(...).then(...) with no error handling so failures are silent; update copyToClipboard to attach a .catch() to navigator.clipboard.writeText to handle rejected promises, and in the catch provide user feedback (e.g., change the button icon to an error state like 'bi-exclamation' or show a temporary tooltip/alert) and optionally attempt a fallback copy method (document.execCommand('copy') using a temporary textarea) before showing the error; keep the existing success flow (icon -> bi-check -> revert) and ensure any icon/state changes are reverted after a timeout.backend/leads/serializers.py-29-33 (1)
29-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve exception chain when re-raising.
The exception is re-raised without chaining (
from exc), which loses the original traceback and makes debugging harder. Python 3 best practice is to preserve the exception context.🔗 Proposed fix to preserve exception chain
def validate_domain(self, value): try: return validate_domain(value) except DjangoValidationError as exc: - raise serializers.ValidationError(exc.messages) + raise serializers.ValidationError(exc.messages) from exc🤖 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 `@backend/leads/serializers.py` around lines 29 - 33, The validate_domain serializer method currently catches DjangoValidationError and re-raises serializers.ValidationError without preserving the original exception chain; update the except block in validate_domain to re-raise the ValidationError using exception chaining (raise serializers.ValidationError(exc.messages) from exc) so the original traceback from DjangoValidationError is preserved for debugging while keeping the existing message extraction.backend/campaigns/serializers.py-228-231 (1)
228-231:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark
usage_countas read-only to prevent client manipulation.The
usage_countfield is currently writable, allowing clients to set arbitrary values during create/update operations. This could lead to inaccurate tracking of template usage.🔒 Proposed fix to mark usage_count as read-only
class EmailTemplateSerializer(serializers.ModelSerializer): class Meta: model = EmailTemplate fields = ['id', 'name', 'subject', 'body', 'category', 'usage_count', 'created_at'] + read_only_fields = ['id', 'usage_count', 'created_at']🤖 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 `@backend/campaigns/serializers.py` around lines 228 - 231, EmailTemplateSerializer currently exposes usage_count as writable; update the serializer to make this field read-only so clients cannot set it. In the EmailTemplateSerializer Meta, either remove usage_count from writable fields and add read_only_fields = ['usage_count'] or explicitly declare usage_count as a ReadOnlyField on the serializer; reference the EmailTemplateSerializer class and its Meta to implement the change.backend/campaigns/views.py-49-74 (1)
49-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen
lead_idsvalidation and consider transactional safety.The validation on line 54 uses
isinstance(lead_ids, list)which will reject tuple inputs that would otherwise be valid iterables. More importantly, the loop processes deletions one-by-one without a transaction wrapper, so a failure midway through will leave the operation in a partially completed state.🛡️ Proposed fix for validation and transaction safety
+from django.db import transaction + `@action`(detail=True, methods=['post']) def unenroll(self, request, pk=None): campaign = self.get_object() lead_ids = request.data.get('lead_ids', []) - if not isinstance(lead_ids, list) or not lead_ids: + if not lead_ids or not hasattr(lead_ids, '__iter__'): return Response( {"error": "lead_ids must be a non-empty list."}, status=status.HTTP_400_BAD_REQUEST, ) unenrolled_count = 0 + with transaction.atomic(): for lead_id in lead_ids: try: lead = Lead.objects.get(id=lead_id, organization=request.user.organization) except Lead.DoesNotExist: continue deleted_count, _ = CampaignLead.objects.filter(campaign=campaign, lead=lead).delete() if deleted_count: unenrolled_count += 1 return Response( {"message": f"Successfully unenrolled {unenrolled_count} leads."}, status=status.HTTP_200_OK, )🤖 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 `@backend/campaigns/views.py` around lines 49 - 74, The unenroll action currently requires a list and deletes per-item without a transaction; change validation to accept any iterable (e.g., list/tuple) by coercing request.data.get('lead_ids') into a list/tuple and validating elements (ints), then fetch valid leads with Lead.objects.filter(id__in=..., organization=request.user.organization) to ensure org-scoped IDs, and perform the deletion inside a django.db.transaction.atomic block using CampaignLead.objects.filter(campaign=campaign, lead__in=valid_leads).delete() to get a single deleted_count; update unenroll to use these symbols (unenroll, Lead, CampaignLead, transaction.atomic) and return the aggregated unenrolled_count.backend/campaigns/views.py-572-596 (1)
572-596:⚠️ Potential issue | 🟡 MinorAvoid repeated DB writes on already-unsubscribed leads (CSRF already covered by middleware)
unsubscribe_viewruns underdjango.middleware.csrf.CsrfViewMiddlewareand there are nocsrf_exempt/csrf_protectoverrides, so CSRF protection should already apply; you don’t need@csrf_protecthere.- Add an idempotency guard before
lead.global_unsubscribe = True/lead.save(...)so repeated POSTs don’t unnecessarily update the row (andupdated_at): iflead.global_unsubscribeis alreadyTrue, return an “Already unsubscribed” page and skip the save.🤖 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 `@backend/campaigns/views.py` around lines 572 - 596, In unsubscribe_view, avoid unnecessary DB writes by checking lead.global_unsubscribe before setting and saving: if lead.global_unsubscribe is already True, return an _unsubscribe_page response like "Already unsubscribed" (HttpResponse with content_type='text/html') and skip lead.save; otherwise set lead.global_unsubscribe = True and call lead.save(update_fields=["global_unsubscribe"]). Also ensure you don't add or require an extra `@csrf_protect` decorator since CsrfViewMiddleware already applies CSRF protection.
🧹 Nitpick comments (7)
frontend/404.html (1)
7-14: 💤 Low valueInconsistent asset path style across auth pages.
This file uses absolute paths (
/theme-boot.js,/theme.css) whilelogin.htmlandregister.htmlwere updated to use relative paths (./theme-boot.js,./theme.css). Consider using consistent path styles across all standalone pages.🤖 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 `@frontend/404.html` around lines 7 - 14, Update the asset references in the 404 page to use the same relative path style as the auth pages: replace the absolute paths '/theme-boot.js' and '/theme.css' used in the <script src="..."> and <link href="..."> tags with relative paths (e.g., './theme-boot.js' and './theme.css') so the page uses consistent asset resolution alongside login.html and register.html.frontend/theme.css (2)
759-769: 💤 Low valueDuplicate
.dashboard-empty-staterule and commented-out code.There's a commented-out
.empty-stateblock (lines 759-764) followed by a duplicate.dashboard-empty-statedefinition (lines 765-769). The first.dashboard-empty-stateappears at lines 754-758. Remove the commented-out code and the duplicate rule.-.analytics-empty-state { - border: none !important; - background: transparent !important; -} - -.dashboard-empty-state { - border: none !important; - background: transparent !important; - padding: 0 !important; -} -/* .empty-state { - background: var(--panel); - border: 1px dashed var(--line); - border-radius: 14px; - padding: 2rem; -} */ - .dashboard-empty-state { - border: none; - background: transparent; - padding: 0; -} +.analytics-empty-state { + border: none !important; + background: transparent !important; +} + +.dashboard-empty-state { + border: none !important; + background: transparent !important; + padding: 0 !important; +}🤖 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 `@frontend/theme.css` around lines 759 - 769, Remove the dead/commented .empty-state block and the duplicate .dashboard-empty-state rule so only one .dashboard-empty-state definition remains; locate the commented .empty-state and the two .dashboard-empty-state declarations in theme.css, delete the commented .empty-state block and the later duplicate .dashboard-empty-state stanza, and keep the original .dashboard-empty-state styling (the first occurrence) to avoid redundant/contradictory rules.
570-570: 💤 Low valueRemove quotes around font-family name.
Stylelint reports that
"Sora"should not be quoted since it's a single-word font name without special characters.-.overview-value { - font-family: 'Sora', sans-serif; +.overview-value { + font-family: Sora, sans-serif;🤖 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 `@frontend/theme.css` at line 570, The font-family declaration uses quoted name 'Sora'; update the font-family line in frontend/theme.css (the rule containing font-family: 'Sora', sans-serif;) to remove the quotes so it reads font-family: Sora, sans-serif; to satisfy Stylelint and single-word font-name conventions.Source: Linters/SAST tools
frontend/main.js (1)
279-281: 💤 Low valueRedundant retry logic for modal injection.
injectShortcutsModalis called on line 294, and then unconditionally again viasetTimeouton line 297. InsideinjectShortcutsModal, there's also a recursivesetTimeoutretry at line 280 if the sidebar isn't found. This results in multiple redundant calls.Consider simplifying to a single retry mechanism:
// Run the DOM injection immediately injectShortcutsModal(); - - // Backup injection: If elements were slow to render, try again in a split second - setTimeout(injectShortcutsModal, 200);The internal retry at line 280 already handles the case where the sidebar isn't available yet.
Also applies to: 294-297
🤖 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 `@frontend/main.js` around lines 279 - 281, injectShortcutsModal is being invoked multiple times: once directly and again via an external setTimeout, while injectShortcutsModal already performs its own recursive setTimeout retry when the sidebar element is not present. Remove the redundant external retry/invocation (the standalone call and the outer setTimeout that calls injectShortcutsModal) so only a single initial call to injectShortcutsModal remains and its internal retry logic handles waiting for the sidebar; keep the internal recursive setTimeout inside injectShortcutsModal unchanged.frontend/analytics.html (1)
143-143: 💤 Low valueRemove commented-out HTML.
Line 143 contains a commented-out
<div>that was replaced by the new empty-state markup. This dead code should be removed to avoid confusion.- <!-- <div id="analytics-empty-state" class="empty-state text-center d-none mt-3"> --> <div id="analytics-empty-state"🤖 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 `@frontend/analytics.html` at line 143, Remove the dead commented-out HTML by deleting the commented line containing the old empty-state div (the commented "<div id="analytics-empty-state" class="empty-state text-center d-none mt-3">") so only the new empty-state markup remains; ensure no leftover comment markers or partial tags remain around the analytics empty-state section.backend/users/tests.py (1)
63-68: ⚡ Quick winMissing authorization test coverage for organization deletion.
The test verifies basic deletion behavior but doesn't cover:
- Non-admin users attempting to delete the organization (should be forbidden)
- Deletion behavior when multiple users exist in the organization
Consider adding test cases for these scenarios to ensure proper authorization enforcement.
📋 Suggested additional test cases
def test_delete_organization_requires_admin_role(self): # Create a non-admin user in the same organization non_admin = User.objects.create_user( email='member@example.com', password='StrongPass123!', organization=self.organization, role='MEMBER', ) self.client.force_authenticate(non_admin) response = self.client.delete('/api/v1/auth/delete-organization/') self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) self.assertTrue(Organization.objects.filter(id=self.organization.id).exists())🤖 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 `@backend/users/tests.py` around lines 63 - 68, Add coverage for authorization and multi-user deletion to the existing test suite: create a new test (e.g., test_delete_organization_requires_admin_role) that creates a non-admin User in the same Organization, force-authenticates that user with self.client.force_authenticate, calls the same DELETE endpoint used in test_delete_organization_removes_current_organization, and assert a 403 response and that Organization/Users still exist; also add a test for deletion when multiple users exist (create an extra admin and a member via User.objects.create_user on the same Organization, authenticate the admin, call DELETE, assert 200 and that Organization and all related Users are removed), referencing the existing test_delete_organization_removes_current_organization, Organization, and User to locate where to add these cases.backend/campaigns/utils.py (1)
3-3: ⚡ Quick winAdd a salt to the Signer for better security isolation.
Using a salt namespaces the unsubscribe tokens and prevents potential cross-usage with other signed data in the application. This is a Django security best practice.
🔒 Proposed fix
-signer = Signer() +signer = Signer(salt='unsubscribe-token')🤖 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 `@backend/campaigns/utils.py` at line 3, The Signer instance defined as signer = Signer() lacks a salt, which can allow tokens to be reused across different signing contexts; update the Signer construction to include a unique salt string (e.g., salt="unsubscribe" or similar) when instantiating Signer and ensure any code that verifies or unsigns tokens uses the same salt value so signing and verification remain consistent (reference the signer variable and any functions that call signer.sign / signer.unsign to update both creation and verification paths).
🤖 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 `@backend/campaigns/views.py`:
- Around line 554-596: The unsubscribe_view embeds request.path directly into an
HTML form action which is an XSS risk; update the code that builds the form
string in unsubscribe_view to HTML-escape the path (e.g., use
django.utils.html.escape on request.path) before interpolation, so the form
action uses the escaped value; ensure the change is applied where form is
constructed and keep the rest of the flow (CSRF token, _unsubscribe_page usage,
and response content_type) unchanged.
In `@backend/tenants/models.py`:
- Line 10: The gemini_api_key field is stored in plaintext; change its type to
an encrypted field (e.g., use EncryptedCharField or EncryptedField provided by
your chosen library such as django-cryptography) in the Tenant model (replace
the existing gemini_api_key = models.CharField(...) declaration),
install/configure the encryption package (pip install django-cryptography or
similar), add the required encryption key setting (FIELD_ENCRYPTION_KEY or
equivalent) in settings from a secure secret source, and create+apply a
migration to convert the column so the API key is encrypted at rest.
In `@backend/users/views.py`:
- Around line 45-48: The handler that writes
request.user.organization.gemini_api_key currently reads the raw gemini_api_key
from the request payload (assigned to request.user.organization.gemini_api_key
and saved) which risks exposing secrets in logs; update the handler to: 1) avoid
logging the raw payload or the gemini_api_key value anywhere (remove any
debug/info logs that include the payload and sanitize request logging paths), 2)
explicitly redact the gemini_api_key before any error/reporting (never include
request.data['gemini_api_key'] in messages), 3) consider changing the endpoint
semantics from PATCH to a credential-specific POST or PUT for rotating/setting
keys and update docs to state this endpoint handles secrets, and 4) keep the
existing assignment/update of request.user.organization.gemini_api_key and
updates_made logic but ensure only the redacted or boolean presence is logged.
- Around line 70-76: The delete_organization action currently allows any
IsAuthenticated user to hard-delete the org; change delete_organization to
authorize only ADMIN users by checking the caller's role (e.g.,
request.user.role or request.user.is_admin) before performing deletion,
returning a 403 if not ADMIN; instead of immediate hard delete on
request.user.organization, implement a safer flow in delete_organization such as
marking the Organization with a deletion flag/status (soft-delete or
"pending_deletion") or require an explicit confirmation parameter and
schedule/queue the actual deletion, and after flagging/scheduling, invalidate
the user's session/token (log out) to avoid the broken state where
request.user.organization no longer exists. Ensure all references use the
delete_organization method and request.user.organization and add appropriate
audit logging for who requested the deletion.
In `@frontend/campaign-builder.html`:
- Line 1147: Remove the duplicate module import so the app isn't initialized
twice: locate the <script type="module" src="./main.js"> tag (the relative
import) in frontend/campaign-builder.html and delete it, leaving only the
absolute import <script type="module" src="https://github.com/main.js"> (the one currently at the
other location) to preserve consistent absolute paths and avoid
double-initialization of the app shell, shortcuts, and event listeners.
- Around line 955-963: The breadcrumb <nav> block is outside the <body> and must
be moved inside it: remove the <nav aria-label="breadcrumb" class="container
mt-3">…</nav> from before the <body class="builder-page"> and paste it
immediately after the opening <body class="builder-page"> tag (or ensure the
first element inside the body is the breadcrumb). Verify there is only one
<body> tag and that the <nav class="breadcrumb"> and its <ol> remain intact.
In `@frontend/login.html`:
- Around line 119-128: The password toggle button (id "floatingPasswordToggle")
is rendered but has no click handler; wire it to the password input (id
"floatingPassword") by adding a click event listener that toggles the input.type
between "password" and "text", updates aria-pressed and aria-label on the
button, and swaps the icon class (e.g., "bi bi-eye" ↔ "bi bi-eye-slash") so the
UI and accessibility state remain correct; place this code in the inline module
script after the form submit handler and guard with checks for both elements
before attaching the listener.
- Line 33: Replace the invalid CSS declaration "min-vh-100: 100vh;" by using the
correct CSS property name—change the property "min-vh-100" to "min-height" so
the rule reads as min-height: 100vh; (locate the declaration that currently uses
the symbol min-vh-100 and update it to use min-height).
- Around line 147-149: login.html currently uses relative module entrypoints
(script src="./main.js" and import './api.js') but campaign-builder.html uses an
absolute path (script src="https://github.com/main.js"); update any absolute module imports to
route-safe relative paths and verify internal imports are relative as well.
Specifically, replace the absolute import example (script src="https://github.com/main.js" in
campaign-builder.html) with a relative path matching other pages (e.g.,
"./main.js" or appropriate relative traversal), and audit modules referenced
from login.html (import { login } from './api.js') to ensure their paths remain
relative and resolve correctly when the app is hosted under a non-root base
path. After changes, test loading both pages from a nested route to confirm
modules load without 404s.
- Around line 102-103: The HTML has an extra closing </div> that unbalances the
form markup and breaks the DOM; locate the closing tags around the login form
(the </form> and its surrounding container/div blocks) and remove the redundant
</div> so that each opened <div> and the <form> have matching closing tags
(ensure elements referenced in the template such as the login form block are
properly nested and validated after removing the extra closing tag).
- Line 19: There is a stray merge artifact "main" inside the HTML head that
breaks the document; open the head section around the <meta name="viewport" ...>
line and remove the standalone "main" token (and any other leftover conflict
markers like <<<<<<<, =======, >>>>>>> if present) so the head contains only
valid tags and content (no extraneous text).
In `@frontend/register.html`:
- Around line 135-144: The password toggle button is missing an event handler;
inside the existing inline module script's DOMContentLoaded handler, wire up the
button with id "passwordToggle" to toggle the input with id "password" between
type "password" and "text", update aria-pressed and aria-label accordingly, and
swap the icon classes (e.g., between "bi bi-eye" and "bi bi-eye-slash"); locate
the DOMContentLoaded block in the register page and add the click listener
referencing "passwordToggle" and "password" to make the control functional and
accessible.
In `@frontend/settings.html`:
- Around line 198-266: Both showConfirmModal and showPromptModal are vulnerable
to XSS because title, message and placeholder are interpolated directly into
HTML; fix by escaping or avoiding HTML interpolation: add a small
escapeHtml(str) helper that replaces &,<,>,",',/ (or build the modal via DOM
APIs) and use it for every injected value (title, message, placeholder) before
inserting modalHtml, or construct the modal with document.createElement and set
textContent/placeholder attributes for elements (update references to
dynamicConfirmBtn, dynamicPromptConfirmBtn, dynamicPromptInput accordingly).
- Around line 323-327: Remove the client-side "Nothing to update" special-case
check that uses payload, gemini_api_key and enable_ai_personalization: delete or
disable the if block that checks "Object.keys(payload).length === 0 &&
gemini_api_key === '' && enable_ai_personalization === true" and the
alert/return so the form submission proceeds to the backend (which already
handles no-op updates) and allow intentional clearing/toggling of the Gemini API
key and AI personalization to be sent to the server.
- Around line 446-467: The click handler for deleteOrgBtn contains an early
"return;" that makes the subsequent delete logic unreachable; remove that stray
"return;" and ensure the flow disables the button and sets textContent to
'Deleting...' before calling fetchWithAuth('/auth/delete-organization/', {
method: 'DELETE' }), then handle the response (res.json(), res.ok), call
clearTokens(), alert, redirect to '/login.html', catch errors, and restore the
button state in the finally block (same pattern used by the delete-leads
handler).
- Around line 388-408: The click handler contains a stray "return;" that makes
everything after it (the deletion logic using deleteLeadsBtn,
fetchWithAuth('/leads/delete-all/'), and the try/catch/finally block)
unreachable; remove that early return (or move it inside a conditional that
should actually short-circuit) so the button disables, shows "Deleting...",
calls fetchWithAuth('/leads/delete-all/') and properly handles response/error
and finally restores the button text and disabled state; ensure you keep
references to deleteLeadsBtn and the existing alert/error handling intact.
In `@frontend/unsubscribe.html`:
- Line 19: Remove the stray merge conflict marker "main" from the HTML file (it
sits inside the head section and breaks the markup); locate the standalone text
token "main" in unsubscribe.html (near the <head> / <meta> area) and delete it
so the head is valid HTML and no stray text remains.
---
Outside diff comments:
In `@frontend/dashboard.html`:
- Around line 191-216: The fetch failure path currently leaves skeleton loaders
visible; update the logic around fetchWithAuth('/analytics/dashboard/') so that
both non-OK responses and exceptions hide the same skeleton elements
('.skeleton-kpi', '`#chart-skeleton`', '`#activity-skeletons`') and reveal an
error/empty-state UI. Specifically, inside the else branch for if (!res.ok) and
inside the catch(e) block, call the same DOM updates you do on success to hide
skeletons and show fallback elements (e.g., hide '.skeleton-kpi' and
'`#chart-skeleton`', hide '`#activity-skeletons`', show '.kpi-value-text' or
'`#activity-list`' as appropriate) and display an error element (e.g.,
'`#dashboard-error`' or create one) with a user-friendly message; make the change
around the fetchWithAuth usage and the DOM selectors used in the success path to
ensure consistent UI in all paths.
In `@frontend/leads.html`:
- Around line 230-249: The template that sets tbody.innerHTML uses unescaped
lead data (see the leads.map(...) template) which allows XSS via lead.email and
other fields; add or reuse an escapeHtml(string) helper (as in campaigns.html)
and apply it to all interpolated user-controlled values (e.g., lead.email,
lead.first_name, lead.last_name, lead.phone, lead.company, lead.score, and
lead.global_unsubscribe output) before inserting into the template and also use
the escaped value for the data-email attribute to ensure no HTML/JS can be
injected.
---
Minor comments:
In `@backend/campaigns/serializers.py`:
- Around line 228-231: EmailTemplateSerializer currently exposes usage_count as
writable; update the serializer to make this field read-only so clients cannot
set it. In the EmailTemplateSerializer Meta, either remove usage_count from
writable fields and add read_only_fields = ['usage_count'] or explicitly declare
usage_count as a ReadOnlyField on the serializer; reference the
EmailTemplateSerializer class and its Meta to implement the change.
In `@backend/campaigns/views.py`:
- Around line 49-74: The unenroll action currently requires a list and deletes
per-item without a transaction; change validation to accept any iterable (e.g.,
list/tuple) by coercing request.data.get('lead_ids') into a list/tuple and
validating elements (ints), then fetch valid leads with
Lead.objects.filter(id__in=..., organization=request.user.organization) to
ensure org-scoped IDs, and perform the deletion inside a
django.db.transaction.atomic block using
CampaignLead.objects.filter(campaign=campaign, lead__in=valid_leads).delete() to
get a single deleted_count; update unenroll to use these symbols (unenroll,
Lead, CampaignLead, transaction.atomic) and return the aggregated
unenrolled_count.
- Around line 572-596: In unsubscribe_view, avoid unnecessary DB writes by
checking lead.global_unsubscribe before setting and saving: if
lead.global_unsubscribe is already True, return an _unsubscribe_page response
like "Already unsubscribed" (HttpResponse with content_type='text/html') and
skip lead.save; otherwise set lead.global_unsubscribe = True and call
lead.save(update_fields=["global_unsubscribe"]). Also ensure you don't add or
require an extra `@csrf_protect` decorator since CsrfViewMiddleware already
applies CSRF protection.
In `@backend/leads/serializers.py`:
- Around line 29-33: The validate_domain serializer method currently catches
DjangoValidationError and re-raises serializers.ValidationError without
preserving the original exception chain; update the except block in
validate_domain to re-raise the ValidationError using exception chaining (raise
serializers.ValidationError(exc.messages) from exc) so the original traceback
from DjangoValidationError is preserved for debugging while keeping the existing
message extraction.
In `@CODE_OF_CONDUCT.md`:
- Around line 50-55: Remove the duplicated "Reporting Guidelines" heading so
only one remains and update the plain URL to a proper Markdown link; locate the
"Reporting Guidelines" heading in CODE_OF_CONDUCT.md and replace the line
containing the raw URL "(https://github.com/Kuldeeep18/LeadOrbit/discussions)"
with a clickable Markdown link such as "[project
discussions](https://github.com/Kuldeeep18/LeadOrbit/discussions)" (or similar
descriptive text) and ensure the contact mention "**`@Kuldeeep18`**" and
surrounding sentence remain intact and grammatically correct.
In `@frontend/analytics.html`:
- Line 6: The head of frontend/analytics.html contains duplicate <title>
elements; remove the redundant <title> so there is exactly one <title> in the
<head> (keep the intended text "LeadOrbit - Analytics" or consolidate any
differing text into that single tag) and ensure no other <title> tags remain in
the same document head (look for the <title> tags in the file to locate and
fix).
In `@frontend/campaigns.html`:
- Line 6: The head contains duplicate <title> elements (both set to "LeadOrbit -
Campaigns"); remove the redundant <title> so there is exactly one <title>
element in the <head> (keep the correct "LeadOrbit - Campaigns" title) and
ensure any other metadata remains intact; also scan other pages for the same
duplicate <title> pattern and fix them similarly.
In `@frontend/dashboard.html`:
- Line 6: There are two <title> elements inside the document head; remove the
duplicate so only one <title> element remains (keep the intended "LeadOrbit -
Dashboard" title), by deleting the extra <title> tag found in the <head> section
and ensuring only a single <title> element exists within the <head>.
In `@frontend/leads.html`:
- Line 6: The head of leads.html contains duplicate <title> elements; remove the
redundant <title> so there is only a single <title> ("LeadOrbit - Lead
Management") in the document head (locate both <title> occurrences and delete
the duplicate), ensuring the retained title is the intended one and that no
other head metadata is accidentally removed.
- Around line 186-198: The copyToClipboard function currently calls
navigator.clipboard.writeText(...).then(...) with no error handling so failures
are silent; update copyToClipboard to attach a .catch() to
navigator.clipboard.writeText to handle rejected promises, and in the catch
provide user feedback (e.g., change the button icon to an error state like
'bi-exclamation' or show a temporary tooltip/alert) and optionally attempt a
fallback copy method (document.execCommand('copy') using a temporary textarea)
before showing the error; keep the existing success flow (icon -> bi-check ->
revert) and ensure any icon/state changes are reverted after a timeout.
In `@frontend/login.html`:
- Line 6: Remove the duplicate <title> tag so only one remains in the document:
locate the two occurrences of the <title> element in the login HTML and delete
the redundant one (keep the intended page title "LeadOrbit - Login" in a single
<title> element), ensuring the head contains only a single <title> tag.
In `@frontend/register.html`:
- Line 6: There are two duplicate <title> elements in register.html; remove one
so only a single <title> remains in the document head (keep the intended text
"LeadOrbit - Register"), ensuring any metadata or script references remain
unchanged—look for the duplicate <title> tags and delete the redundant one.
In `@frontend/settings.html`:
- Line 6: There are two <title> elements in the <head> for this page causing
duplicate document titles; remove the redundant <title> so only a single <title>
"LeadOrbit - Settings" remains within the <head> section (ensure any metadata or
templating that generates the second <title> is removed or consolidated so only
one <title> tag is present).
In `@frontend/unsubscribe.html`:
- Line 6: Remove the duplicate <title> element in the document head so only a
single <title> tag remains; locate the two <title> tags (the one shown in the
diff and the other later in the head) and delete one of them, keeping the
correct/desired page title text within the remaining <title> element.
In `@README.md`:
- Around line 23-30: Remove the duplicated README content by keeping a single
canonical copy (retain the headings "## Code of Conduct" and "## Contributing")
and delete the redundant block; replace the placeholder "## Contributing" stub
with a real pointer to the contribution guide (for example: reference
CONTRIBUTING.md or the project's contribution URL and brief steps for
PRs/issues), and ensure the removed duplicate content (the other copy later in
the file) is fully deleted so only one complete README remains.
---
Nitpick comments:
In `@backend/campaigns/utils.py`:
- Line 3: The Signer instance defined as signer = Signer() lacks a salt, which
can allow tokens to be reused across different signing contexts; update the
Signer construction to include a unique salt string (e.g., salt="unsubscribe" or
similar) when instantiating Signer and ensure any code that verifies or unsigns
tokens uses the same salt value so signing and verification remain consistent
(reference the signer variable and any functions that call signer.sign /
signer.unsign to update both creation and verification paths).
In `@backend/users/tests.py`:
- Around line 63-68: Add coverage for authorization and multi-user deletion to
the existing test suite: create a new test (e.g.,
test_delete_organization_requires_admin_role) that creates a non-admin User in
the same Organization, force-authenticates that user with
self.client.force_authenticate, calls the same DELETE endpoint used in
test_delete_organization_removes_current_organization, and assert a 403 response
and that Organization/Users still exist; also add a test for deletion when
multiple users exist (create an extra admin and a member via
User.objects.create_user on the same Organization, authenticate the admin, call
DELETE, assert 200 and that Organization and all related Users are removed),
referencing the existing test_delete_organization_removes_current_organization,
Organization, and User to locate where to add these cases.
In `@frontend/404.html`:
- Around line 7-14: Update the asset references in the 404 page to use the same
relative path style as the auth pages: replace the absolute paths
'/theme-boot.js' and '/theme.css' used in the <script src="..."> and <link
href="..."> tags with relative paths (e.g., './theme-boot.js' and './theme.css')
so the page uses consistent asset resolution alongside login.html and
register.html.
In `@frontend/analytics.html`:
- Line 143: Remove the dead commented-out HTML by deleting the commented line
containing the old empty-state div (the commented "<div
id="analytics-empty-state" class="empty-state text-center d-none mt-3">") so
only the new empty-state markup remains; ensure no leftover comment markers or
partial tags remain around the analytics empty-state section.
In `@frontend/main.js`:
- Around line 279-281: injectShortcutsModal is being invoked multiple times:
once directly and again via an external setTimeout, while injectShortcutsModal
already performs its own recursive setTimeout retry when the sidebar element is
not present. Remove the redundant external retry/invocation (the standalone call
and the outer setTimeout that calls injectShortcutsModal) so only a single
initial call to injectShortcutsModal remains and its internal retry logic
handles waiting for the sidebar; keep the internal recursive setTimeout inside
injectShortcutsModal unchanged.
In `@frontend/theme.css`:
- Around line 759-769: Remove the dead/commented .empty-state block and the
duplicate .dashboard-empty-state rule so only one .dashboard-empty-state
definition remains; locate the commented .empty-state and the two
.dashboard-empty-state declarations in theme.css, delete the commented
.empty-state block and the later duplicate .dashboard-empty-state stanza, and
keep the original .dashboard-empty-state styling (the first occurrence) to avoid
redundant/contradictory rules.
- Line 570: The font-family declaration uses quoted name 'Sora'; update the
font-family line in frontend/theme.css (the rule containing font-family: 'Sora',
sans-serif;) to remove the quotes so it reads font-family: Sora, sans-serif; to
satisfy Stylelint and single-word font-name conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+554
to
+596
| def unsubscribe_view(request, lead_id, token): | ||
| """Public unsubscribe endpoint for GDPR/CAN-SPAM compliance.""" | ||
| verified = verify_unsubscribe_token(token) | ||
|
|
||
| if not verified or str(verified) != str(lead_id): | ||
| return HttpResponse( | ||
| "Invalid unsubscribe link", | ||
| status=400, | ||
| ) | ||
|
|
||
| try: | ||
| lead = Lead.objects.get(id=lead_id) | ||
| except Lead.DoesNotExist: | ||
| return HttpResponse( | ||
| "Lead not found", | ||
| status=404, | ||
| ) | ||
|
|
||
| if request.method != 'POST': | ||
| csrf_token = get_token(request) | ||
| form = ( | ||
| f'<form method="post" action="{request.path}">' | ||
| f'<input type="hidden" name="csrfmiddlewaretoken" value="{csrf_token}">' | ||
| '<button type="submit">Confirm unsubscribe</button>' | ||
| '</form>' | ||
| ) | ||
| html = _unsubscribe_page( | ||
| 'Confirm unsubscribe', | ||
| 'Please confirm that you want to unsubscribe from future emails sent through LeadOrbit.', | ||
| form, | ||
| ) | ||
| return HttpResponse(html, content_type='text/html') | ||
|
|
||
| lead.global_unsubscribe = True | ||
| lead.save(update_fields=["global_unsubscribe"]) | ||
|
|
||
| html = _unsubscribe_page( | ||
| 'Unsubscribed', | ||
| 'You have been unsubscribed from all future emails sent through LeadOrbit.', | ||
| '<p>If you received this link by mistake, no further action is needed.</p>', | ||
| ) | ||
|
|
||
| return HttpResponse(html, content_type='text/html') |
There was a problem hiding this comment.
XSS risk: request.path embedded in HTML without escaping.
Line 575 constructs an HTML form action attribute by directly interpolating request.path into the template string without escaping. Although Django's URL routing constrains the path format, this pattern violates defense-in-depth principles and could be exploited if routing rules change or if there are unexpected characters in UUIDs/tokens.
🔒 Proposed fix to escape request.path
from django.http import HttpResponse
from django.middleware.csrf import get_token
+from django.utils.html import escape
from leads.models import Lead
from .utils import verify_unsubscribe_token
def unsubscribe_view(request, lead_id, token):
"""Public unsubscribe endpoint for GDPR/CAN-SPAM compliance."""
verified = verify_unsubscribe_token(token)
if not verified or str(verified) != str(lead_id):
return HttpResponse(
"Invalid unsubscribe link",
status=400,
)
try:
lead = Lead.objects.get(id=lead_id)
except Lead.DoesNotExist:
return HttpResponse(
"Lead not found",
status=404,
)
if request.method != 'POST':
csrf_token = get_token(request)
form = (
- f'<form method="post" action="{request.path}">'
+ f'<form method="post" action="{escape(request.path)}">'
f'<input type="hidden" name="csrfmiddlewaretoken" value="{csrf_token}">'
'<button type="submit">Confirm unsubscribe</button>'
'</form>'
)
html = _unsubscribe_page(
'Confirm unsubscribe',
'Please confirm that you want to unsubscribe from future emails sent through LeadOrbit.',
form,
)
return HttpResponse(html, content_type='text/html')
lead.global_unsubscribe = True
lead.save(update_fields=["global_unsubscribe"])
html = _unsubscribe_page(
'Unsubscribed',
'You have been unsubscribed from all future emails sent through LeadOrbit.',
'<p>If you received this link by mistake, no further action is needed.</p>',
)
return HttpResponse(html, content_type='text/html')🧰 Tools
🪛 ast-grep (0.43.0)
[error] 558-561: Lack of sanitization of user data
Context: HttpResponse(
"Invalid unsubscribe link",
status=400,
)
Note: [CWE-20].
(http-response-from-request)
[error] 566-569: Lack of sanitization of user data
Context: HttpResponse(
"Lead not found",
status=404,
)
Note: [CWE-20].
(http-response-from-request)
[error] 584-584: Lack of sanitization of user data
Context: HttpResponse(html, content_type='text/html')
Note: [CWE-20].
(http-response-from-request)
[error] 595-595: Lack of sanitization of user data
Context: HttpResponse(html, content_type='text/html')
Note: [CWE-20].
(http-response-from-request)
🤖 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 `@backend/campaigns/views.py` around lines 554 - 596, The unsubscribe_view
embeds request.path directly into an HTML form action which is an XSS risk;
update the code that builds the form string in unsubscribe_view to HTML-escape
the path (e.g., use django.utils.html.escape on request.path) before
interpolation, so the form action uses the escaped value; ensure the change is
applied where form is constructed and keep the rest of the flow (CSRF token,
_unsubscribe_page usage, and response content_type) unchanged.
| name = models.CharField(max_length=255) | ||
| billing_plan = models.CharField(max_length=50, default='FREE') | ||
| created_at = models.DateTimeField(auto_now_add=True) | ||
| gemini_api_key = models.CharField(max_length=255, blank=True, null=True) |
There was a problem hiding this comment.
API key stored in plaintext is a critical security risk.
Storing gemini_api_key as plaintext in the database exposes it if the database is compromised (backup leak, SQL injection, insider threat). Django provides encryption utilities (django.db.models.fields.EncryptedField from third-party packages like django-encrypted-model-fields or django-cryptography) to encrypt sensitive fields at rest.
🔐 Recommended approach
Install django-cryptography or similar:
pip install django-cryptographyThen update the model:
+from encrypted_model_fields.fields import EncryptedCharField
class Organization(models.Model):
id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
name = models.CharField(max_length=255)
billing_plan = models.CharField(max_length=50, default='FREE')
created_at = models.DateTimeField(auto_now_add=True)
- gemini_api_key = models.CharField(max_length=255, blank=True, null=True)
+ gemini_api_key = EncryptedCharField(max_length=255, blank=True, null=True)
enable_ai_personalization = models.BooleanField(default=True)Ensure FIELD_ENCRYPTION_KEY is configured in settings and stored securely (environment variable, secrets manager).
🤖 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 `@backend/tenants/models.py` at line 10, The gemini_api_key field is stored in
plaintext; change its type to an encrypted field (e.g., use EncryptedCharField
or EncryptedField provided by your chosen library such as django-cryptography)
in the Tenant model (replace the existing gemini_api_key = models.CharField(...)
declaration), install/configure the encryption package (pip install
django-cryptography or similar), add the required encryption key setting
(FIELD_ENCRYPTION_KEY or equivalent) in settings from a secure secret source,
and create+apply a migration to convert the column so the API key is encrypted
at rest.
Comment on lines
+45
to
+48
| if gemini_api_key is not None: | ||
| request.user.organization.gemini_api_key = str(gemini_api_key).strip() or None | ||
| request.user.organization.save(update_fields=['gemini_api_key']) | ||
| updates_made = True |
There was a problem hiding this comment.
API key in request payload may be exposed in logs.
If request logging is enabled (common in production for debugging), the gemini_api_key from payload.get('gemini_api_key') may appear in access logs or error traces. Consider:
- Sanitizing logs to exclude sensitive fields
- Documenting that this endpoint handles secrets
- Using POST/PUT for key updates rather than PATCH (semantically clearer for credential rotation)
🤖 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 `@backend/users/views.py` around lines 45 - 48, The handler that writes
request.user.organization.gemini_api_key currently reads the raw gemini_api_key
from the request payload (assigned to request.user.organization.gemini_api_key
and saved) which risks exposing secrets in logs; update the handler to: 1) avoid
logging the raw payload or the gemini_api_key value anywhere (remove any
debug/info logs that include the payload and sanitize request logging paths), 2)
explicitly redact the gemini_api_key before any error/reporting (never include
request.data['gemini_api_key'] in messages), 3) consider changing the endpoint
semantics from PATCH to a credential-specific POST or PUT for rotating/setting
keys and update docs to state this endpoint handles secrets, and 4) keep the
existing assignment/update of request.user.organization.gemini_api_key and
updates_made logic but ensure only the redacted or boolean presence is logged.
Comment on lines
+70
to
+76
| @action(detail=False, methods=['delete'], permission_classes=[IsAuthenticated], url_path='delete-organization') | ||
| def delete_organization(self, request): | ||
| request.user.organization.delete() | ||
| return Response( | ||
| {'message': 'Organization successfully deleted.'}, | ||
| status=status.HTTP_200_OK, | ||
| ) |
There was a problem hiding this comment.
Missing authorization check allows any user to delete the entire organization.
The delete_organization endpoint permits any IsAuthenticated user to delete their organization, including all cascading data (leads, campaigns, users). This should be restricted to ADMIN role users only. Additionally:
- Deleting the organization while the user remains authenticated creates a broken state—subsequent requests will fail when accessing
request.user.organization. - No confirmation, backup, or soft-delete mechanism protects against accidental or malicious deletion.
🔒 Proposed authorization fix
`@action`(detail=False, methods=['delete'], permission_classes=[IsAuthenticated], url_path='delete-organization')
def delete_organization(self, request):
+ if request.user.role != 'ADMIN':
+ return Response(
+ {'detail': 'Only organization admins can delete the organization.'},
+ status=status.HTTP_403_FORBIDDEN,
+ )
request.user.organization.delete()
return Response(
{'message': 'Organization successfully deleted.'},
status=status.HTTP_200_OK,
)Additionally, consider logging the user out or marking the org for deletion rather than immediate hard delete.
🤖 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 `@backend/users/views.py` around lines 70 - 76, The delete_organization action
currently allows any IsAuthenticated user to hard-delete the org; change
delete_organization to authorize only ADMIN users by checking the caller's role
(e.g., request.user.role or request.user.is_admin) before performing deletion,
returning a 403 if not ADMIN; instead of immediate hard delete on
request.user.organization, implement a safer flow in delete_organization such as
marking the Organization with a deletion flag/status (soft-delete or
"pending_deletion") or require an explicit confirmation parameter and
schedule/queue the actual deletion, and after flagging/scheduling, invalidate
the user's session/token (log out) to avoid the broken state where
request.user.organization no longer exists. Ensure all references use the
delete_organization method and request.user.organization and add appropriate
audit logging for who requested the deletion.
Comment on lines
+955
to
+963
| <!-- Breadcrumb Navigation --> | ||
| <nav aria-label="breadcrumb" class="container mt-3"> | ||
| <ol class="breadcrumb"> | ||
| <li class="breadcrumb-item"><a href="/dashboard.html">Home</a></li> | ||
| <li class="breadcrumb-item"><a href="/campaigns.html">Campaigns</a></li> | ||
| <li class="breadcrumb-item active" aria-current="page">Builder</li> | ||
| </ol> | ||
| </nav> | ||
| <body class="builder-page"> |
There was a problem hiding this comment.
Invalid HTML structure: <nav> element placed before <body> tag.
The breadcrumb navigation is placed between </head> (implicit, after </style>) and <body>, which is invalid HTML. Elements outside <body> may render incorrectly or be moved by the browser's error correction.
Move the breadcrumb inside the body:
</style>
</head>
-<!-- Breadcrumb Navigation -->
-<nav aria-label="breadcrumb" class="container mt-3">
- <ol class="breadcrumb">
- <li class="breadcrumb-item"><a href="https://github.com/dashboard.html">Home</a></li>
- <li class="breadcrumb-item"><a href="https://github.com/campaigns.html">Campaigns</a></li>
- <li class="breadcrumb-item active" aria-current="page">Builder</li>
- </ol>
-</nav>
<body class="builder-page">
+ <!-- Breadcrumb Navigation -->
+ <nav aria-label="breadcrumb" class="container mt-3">
+ <ol class="breadcrumb">
+ <li class="breadcrumb-item"><a href="https://github.com/dashboard.html">Home</a></li>
+ <li class="breadcrumb-item"><a href="https://github.com/campaigns.html">Campaigns</a></li>
+ <li class="breadcrumb-item active" aria-current="page">Builder</li>
+ </ol>
+ </nav>
<!-- ─── Top Bar ─────────────────────────── -->📝 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.
Suggested change
| <!-- Breadcrumb Navigation --> | |
| <nav aria-label="breadcrumb" class="container mt-3"> | |
| <ol class="breadcrumb"> | |
| <li class="breadcrumb-item"><a href="/dashboard.html">Home</a></li> | |
| <li class="breadcrumb-item"><a href="/campaigns.html">Campaigns</a></li> | |
| <li class="breadcrumb-item active" aria-current="page">Builder</li> | |
| </ol> | |
| </nav> | |
| <body class="builder-page"> | |
| <body class="builder-page"> | |
| <!-- Breadcrumb Navigation --> | |
| <nav aria-label="breadcrumb" class="container mt-3"> | |
| <ol class="breadcrumb"> | |
| <li class="breadcrumb-item"><a href="/dashboard.html">Home</a></li> | |
| <li class="breadcrumb-item"><a href="/campaigns.html">Campaigns</a></li> | |
| <li class="breadcrumb-item active" aria-current="page">Builder</li> | |
| </ol> | |
| </nav> |
🤖 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 `@frontend/campaign-builder.html` around lines 955 - 963, The breadcrumb <nav>
block is outside the <body> and must be moved inside it: remove the <nav
aria-label="breadcrumb" class="container mt-3">…</nav> from before the <body
class="builder-page"> and paste it immediately after the opening <body
class="builder-page"> tag (or ensure the first element inside the body is the
breadcrumb). Verify there is only one <body> tag and that the <nav
class="breadcrumb"> and its <ol> remain intact.
Comment on lines
+198
to
+266
| function showConfirmModal(title, message, confirmLabel, onConfirm) { | ||
| const modalHtml = ` | ||
| <div class="modal fade" id="dynamicConfirmModal" tabindex="-1"> | ||
| <div class="modal-dialog"> | ||
| <div class="modal-content"> | ||
| <div class="modal-header"> | ||
| <h5 class="modal-title">${title}</h5> | ||
| <button type="button" class="btn-close" data-bs-dismiss="modal"></button> | ||
| </div> | ||
| <div class="modal-body"> | ||
| <p>${message}</p> | ||
| </div> | ||
| <div class="modal-footer"> | ||
| <button type="button" class="btn btn-secondary" data-bs-dismiss="modal">Cancel</button> | ||
| <button type="button" class="btn btn-danger" id="dynamicConfirmBtn">${confirmLabel}</button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| `; | ||
|
|
||
| const existingModal = document.getElementById('dynamicConfirmModal'); | ||
| if (existingModal) existingModal.remove(); | ||
|
|
||
| document.body.insertAdjacentHTML('beforeend', modalHtml); | ||
| const modal = new bootstrap.Modal(document.getElementById('dynamicConfirmModal')); | ||
|
|
||
| document.getElementById('dynamicConfirmBtn').onclick = () => { | ||
| modal.hide(); | ||
| if (onConfirm) onConfirm(); | ||
| }; | ||
| modal.show(); | ||
| } | ||
|
|
||
| function showPromptModal(title, message, placeholder, onConfirm) { | ||
| const modalHtml = ` | ||
| <div class="modal fade" id="dynamicPromptModal" tabindex="-1"> | ||
| <div class="modal-dialog"> | ||
| <div class="modal-content"> | ||
| <div class="modal-header"> | ||
| <h5 class="modal-title">${title}</h5> | ||
| <button type="button" class="btn-close" data-bs-dismiss="modal"></button> | ||
| </div> | ||
| <div class="modal-body"> | ||
| <p>${message}</p> | ||
| <input type="text" class="form-control" id="dynamicPromptInput" placeholder="${placeholder}"> | ||
| </div> | ||
| <div class="modal-footer"> | ||
| <button type="button" class="btn btn-secondary" data-bs-dismiss="modal">Cancel</button> | ||
| <button type="button" class="btn btn-danger" id="dynamicPromptConfirmBtn">Confirm</button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| `; | ||
|
|
||
| const existingModal = document.getElementById('dynamicPromptModal'); | ||
| if (existingModal) existingModal.remove(); | ||
|
|
||
| document.body.insertAdjacentHTML('beforeend', modalHtml); | ||
| const modal = new bootstrap.Modal(document.getElementById('dynamicPromptModal')); | ||
|
|
||
| document.getElementById('dynamicPromptConfirmBtn').onclick = () => { | ||
| const inputValue = document.getElementById('dynamicPromptInput').value; | ||
| modal.hide(); | ||
| if (onConfirm) onConfirm(inputValue); | ||
| }; | ||
| modal.show(); | ||
| } |
There was a problem hiding this comment.
XSS vulnerability in dynamic modal helpers.
showConfirmModal and showPromptModal interpolate title, message, and placeholder directly into HTML strings without escaping. If any of these values contain user-controlled content (e.g., the organization name in line 415), an attacker could inject malicious scripts.
For example, if orgName is <img src=x onerror=alert(1)>, the prompt modal would execute JavaScript.
Proposed fix
Add an escape helper and use it for all interpolated values:
+function escapeHtml(str) {
+ return String(str ?? '')
+ .replace(/&/g, '&')
+ .replace(/</g, '<')
+ .replace(/>/g, '>')
+ .replace(/"/g, '"')
+ .replace(/'/g, '&`#39`;');
+}
+
function showConfirmModal(title, message, confirmLabel, onConfirm) {
const modalHtml = `
<div class="modal fade" id="dynamicConfirmModal" tabindex="-1">
<div class="modal-dialog">
<div class="modal-content">
<div class="modal-header">
- <h5 class="modal-title">${title}</h5>
+ <h5 class="modal-title">${escapeHtml(title)}</h5>
<button type="button" class="btn-close" data-bs-dismiss="modal"></button>
</div>
<div class="modal-body">
- <p>${message}</p>
+ <p>${escapeHtml(message)}</p>
</div>Apply the same pattern to showPromptModal for title, message, and placeholder.
🤖 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 `@frontend/settings.html` around lines 198 - 266, Both showConfirmModal and
showPromptModal are vulnerable to XSS because title, message and placeholder are
interpolated directly into HTML; fix by escaping or avoiding HTML interpolation:
add a small escapeHtml(str) helper that replaces &,<,>,",',/ (or build the modal
via DOM APIs) and use it for every injected value (title, message, placeholder)
before inserting modalHtml, or construct the modal with document.createElement
and set textContent/placeholder attributes for elements (update references to
dynamicConfirmBtn, dynamicPromptConfirmBtn, dynamicPromptInput accordingly).
Comment on lines
+323
to
327
| // 1. Run the "Nothing to update" check on the text fields first | ||
| if (Object.keys(payload).length === 0 && gemini_api_key === '' && enable_ai_personalization === true) { | ||
| alert('Nothing to update.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
"Nothing to update" check has flawed logic.
The condition gemini_api_key === '' && enable_ai_personalization === true means the check only triggers when both conditions are met. However:
- If the user intentionally clears the Gemini API key (sets it to empty), this check prevents the update.
- The check doesn't account for whether the current values differ from what's already saved.
This could confuse users who want to remove their API key or toggle AI personalization off while keeping other fields empty.
Consider removing this special-case check and letting the backend handle "no changes" validation (which it already does):
Proposed fix
- // 1. Run the "Nothing to update" check on the text fields first
- if (Object.keys(payload).length === 0 && gemini_api_key === '' && enable_ai_personalization === true) {
- alert('Nothing to update.');
- return;
- }
-
- // 2. Then safely attach the Gemini fields right before sending the PATCH request
- payload.gemini_api_key = gemini_api_key || null;
- payload.enable_ai_personalization = enable_ai_personalization;
+ // Always include AI fields in the payload
+ payload.gemini_api_key = gemini_api_key || null;
+ payload.enable_ai_personalization = enable_ai_personalization;🤖 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 `@frontend/settings.html` around lines 323 - 327, Remove the client-side
"Nothing to update" special-case check that uses payload, gemini_api_key and
enable_ai_personalization: delete or disable the if block that checks
"Object.keys(payload).length === 0 && gemini_api_key === '' &&
enable_ai_personalization === true" and the alert/return so the form submission
proceeds to the backend (which already handles no-op updates) and allow
intentional clearing/toggling of the Gemini API key and AI personalization to be
sent to the server.
Comment on lines
+388
to
+408
| return; | ||
|
|
||
| deleteLeadsBtn.disabled = true; | ||
| deleteLeadsBtn.textContent = 'Deleting...'; | ||
|
|
||
| try { | ||
| const res = await fetchWithAuth('/leads/delete-all/', { method: 'DELETE' }); | ||
| const data = await res.json().catch(() => ({})); | ||
| if (!res.ok) { | ||
| alert(data.detail || data.error || 'Could not delete leads right now.'); | ||
| return; | ||
| } | ||
|
|
||
| alert(data.message || 'All leads were deleted.'); | ||
| } catch (e) { | ||
| alert('Could not delete leads right now.'); | ||
| } finally { | ||
| deleteLeadsBtn.disabled = false; | ||
| deleteLeadsBtn.textContent = 'Delete All Leads'; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Dead code after return statement.
Lines 390-408 are unreachable because return; on line 388 exits the click handler. This appears to be leftover code from before the modal refactor.
Proposed fix
deleteLeadsBtn.addEventListener('click', async () => {
showConfirmModal('Delete All Leads', 'Delete every lead in this organization? This cannot be undone.', 'Yes, Delete', () => {
// Proceed with deletion
// ... modal callback code ...
});
-return;
-
- deleteLeadsBtn.disabled = true;
- deleteLeadsBtn.textContent = 'Deleting...';
-
- try {
- const res = await fetchWithAuth('/leads/delete-all/', { method: 'DELETE' });
- const data = await res.json().catch(() => ({}));
- if (!res.ok) {
- alert(data.detail || data.error || 'Could not delete leads right now.');
- return;
- }
-
- alert(data.message || 'All leads were deleted.');
- } catch (e) {
- alert('Could not delete leads right now.');
- } finally {
- deleteLeadsBtn.disabled = false;
- deleteLeadsBtn.textContent = 'Delete All Leads';
- }
});🤖 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 `@frontend/settings.html` around lines 388 - 408, The click handler contains a
stray "return;" that makes everything after it (the deletion logic using
deleteLeadsBtn, fetchWithAuth('/leads/delete-all/'), and the try/catch/finally
block) unreachable; remove that early return (or move it inside a conditional
that should actually short-circuit) so the button disables, shows "Deleting...",
calls fetchWithAuth('/leads/delete-all/') and properly handles response/error
and finally restores the button text and disabled state; ensure you keep
references to deleteLeadsBtn and the existing alert/error handling intact.
Comment on lines
+446
to
+467
| return; | ||
| deleteOrgBtn.disabled = true; | ||
| deleteOrgBtn.textContent = 'Deleting...'; | ||
|
|
||
| try { | ||
| const res = await fetchWithAuth('/auth/delete-organization/', { method: 'DELETE' }); | ||
| const data = await res.json().catch(() => ({})); | ||
| if (!res.ok) { | ||
| alert(data.detail || data.error || 'Could not delete organization right now.'); | ||
| return; | ||
| } | ||
|
|
||
| clearTokens(); | ||
| alert(data.message || 'Organization deleted. You have been signed out.'); | ||
| window.location.href = '/login.html'; | ||
| } catch (e) { | ||
| alert('Could not delete organization right now.'); | ||
| } finally { | ||
| deleteOrgBtn.disabled = false; | ||
| deleteOrgBtn.textContent = 'Delete Organization'; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Dead code after return statement.
Lines 447-467 are unreachable because return; on line 446 exits the click handler. Same issue as the delete-leads handler.
Proposed fix
});
-return;
- deleteOrgBtn.disabled = true;
- deleteOrgBtn.textContent = 'Deleting...';
-
- try {
- const res = await fetchWithAuth('/auth/delete-organization/', { method: 'DELETE' });
- const data = await res.json().catch(() => ({}));
- if (!res.ok) {
- alert(data.detail || data.error || 'Could not delete organization right now.');
- return;
- }
-
- clearTokens();
- alert(data.message || 'Organization deleted. You have been signed out.');
- window.location.href = '/login.html';
- } catch (e) {
- alert('Could not delete organization right now.');
- } finally {
- deleteOrgBtn.disabled = false;
- deleteOrgBtn.textContent = 'Delete Organization';
- }
});📝 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.
Suggested change
| return; | |
| deleteOrgBtn.disabled = true; | |
| deleteOrgBtn.textContent = 'Deleting...'; | |
| try { | |
| const res = await fetchWithAuth('/auth/delete-organization/', { method: 'DELETE' }); | |
| const data = await res.json().catch(() => ({})); | |
| if (!res.ok) { | |
| alert(data.detail || data.error || 'Could not delete organization right now.'); | |
| return; | |
| } | |
| clearTokens(); | |
| alert(data.message || 'Organization deleted. You have been signed out.'); | |
| window.location.href = '/login.html'; | |
| } catch (e) { | |
| alert('Could not delete organization right now.'); | |
| } finally { | |
| deleteOrgBtn.disabled = false; | |
| deleteOrgBtn.textContent = 'Delete Organization'; | |
| } | |
| }); | |
| deleteOrgBtn.disabled = true; | |
| deleteOrgBtn.textContent = 'Deleting...'; | |
| try { | |
| const res = await fetchWithAuth('/auth/delete-organization/', { method: 'DELETE' }); | |
| const data = await res.json().catch(() => ({})); | |
| if (!res.ok) { | |
| alert(data.detail || data.error || 'Could not delete organization right now.'); | |
| return; | |
| } | |
| clearTokens(); | |
| alert(data.message || 'Organization deleted. You have been signed out.'); | |
| window.location.href = '/login.html'; | |
| } catch (e) { | |
| alert('Could not delete organization right now.'); | |
| } finally { | |
| deleteOrgBtn.disabled = false; | |
| deleteOrgBtn.textContent = 'Delete Organization'; | |
| } | |
| }); |
🤖 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 `@frontend/settings.html` around lines 446 - 467, The click handler for
deleteOrgBtn contains an early "return;" that makes the subsequent delete logic
unreachable; remove that stray "return;" and ensure the flow disables the button
and sets textContent to 'Deleting...' before calling
fetchWithAuth('/auth/delete-organization/', { method: 'DELETE' }), then handle
the response (res.json(), res.ok), call clearTokens(), alert, redirect to
'/login.html', catch errors, and restore the button state in the finally block
(same pattern used by the delete-leads handler).
| <meta name="twitter:description" content="LeadOrbit unsubscribe - Manage your email preferences."> | ||
| <link rel="icon" type="image/png" href="/favicon.png"> | ||
|
|
||
| main |
There was a problem hiding this comment.
Remove merge conflict marker.
Line 19 contains the standalone text main which appears to be a leftover merge conflict marker that breaks the HTML head section.
🔧 Proposed fix
<link rel="icon" type="image/png" href="https://github.com/favicon.png">
- main
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>LeadOrbit - Unsubscribe</title>📝 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.
Suggested change
| main | |
| <link rel="icon" type="image/png" href="/favicon.png"> | |
| <meta charset="UTF-8" /> | |
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | |
| <title>LeadOrbit - Unsubscribe</title> |
🤖 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 `@frontend/unsubscribe.html` at line 19, Remove the stray merge conflict marker
"main" from the HTML file (it sits inside the head section and breaks the
markup); locate the standalone text token "main" in unsubscribe.html (near the
<head> / <meta> area) and delete it so the head is valid HTML and no stray text
remains.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves Kuldeeep18#198.\n\nAdds a campaign unenroll endpoint plus builder UI support so selected leads can be removed from a campaign cleanly. Also adds a workflow test covering the API action.\n\nValidation:\n- /Users/saurabhkumarbajpaiai/.cache/codex-runtimes/codex-primary-runtime/dependencies/python/bin/python3 manage.py test campaigns.tests.CampaignWorkflowTests.test_unenroll_action_removes_selected_leads_from_campaign\n- python3 -m py_compile backend/campaigns/views.py backend/campaigns/tests.py\n- git diff --check\n- chrome headless DOM check confirms the new Unenroll selected control is rendered in the builder
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style