fix: resolve merge conflict markers in leads.html#301
Open
bh462007 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthrough
ChangesLeads Page UI and Script Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR resolves merge-conflict artifacts in leads.html and adds UI + logic for viewing CSV import history details, including an import error log modal.
Changes:
- Removes leftover merge markers/conflict noise and normalizes the layout/whitespace.
- Adds an “Import Errors” modal and client-side rendering for row-level import error logs.
- Introduces import history state (
allImportJobs) and wiring for “View” error buttons.
Comments suppressed due to low confidence (2)
frontend/leads.html:1
- Untrusted server-provided values are interpolated into
innerHTMLwithout escaping (entry.field,entry.message, andjob.filename). This is a concrete XSS vector if any of those fields can contain HTML. UseescapeHtml(...)on these values (or build the rows withtextContent/DOM APIs) before inserting into the DOM.
<!DOCTYPE html>
frontend/leads.html:577
- Untrusted server-provided values are interpolated into
innerHTMLwithout escaping (entry.field,entry.message, andjob.filename). This is a concrete XSS vector if any of those fields can contain HTML. UseescapeHtml(...)on these values (or build the rows withtextContent/DOM APIs) before inserting into the DOM.
tbody.innerHTML = jobs.map(job => `
<tr>
<td class="fw-semibold">${job.filename}</td>
<td>${job.total_rows ?? 0}</td>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
510
to
517
| function openImportErrors(jobId) { | ||
| const job = allImportJobs.find(item => item.id === jobId); | ||
| if (!job) return; | ||
|
|
||
| document.getElementById('import-errors-meta').textContent = | ||
| `${job.filename} · ${job.failed_count} error${job.failed_count === 1 ? '' : 's'}`; | ||
| renderImportErrorRows(job); | ||
| bootstrap.Modal.getOrCreateInstance(document.getElementById('importErrorsModal')).show(); | ||
| } |
Comment on lines
588
to
590
| document.querySelectorAll('.view-import-errors-btn').forEach(btn => { | ||
| btn.addEventListener('click', () => openImportErrors(btn.dataset.jobId)); | ||
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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 (2)
731-745:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t leave infinite loading UI on failed data loads.
Line 733 and Line 741 return silently on non-OK responses, so both tables can stay stuck on spinner rows with no recovery hint.
Suggested fix
async function loadLeads() { - const res = await fetchWithAuth('/leads/'); - if (!res.ok) return; - allLeads = await res.json(); - renderStats(allLeads); - renderLeadRows(allLeads); + const tbody = document.getElementById('leads-table-body'); + const res = await fetchWithAuth('/leads/'); + if (!res.ok) { + tbody.innerHTML = '<tr><td colspan="7" class="text-center py-4 text-danger">Failed to load leads.</td></tr>'; + return; + } + allLeads = await res.json(); + renderStats(allLeads); + renderLeadRows(allLeads); } @@ async function loadImportHistory() { - const res = await fetchWithAuth('/lead-import-jobs/'); - if (!res.ok) return; + const tbody = document.getElementById('import-history-body'); + const res = await fetchWithAuth('/lead-import-jobs/'); + if (!res.ok) { + tbody.innerHTML = '<tr><td colspan="6" class="text-center py-4 text-danger">Failed to load import history.</td></tr>'; + return; + } const payload = await res.json(); allImportJobs = Array.isArray(payload) ? payload : (payload.results || []); renderImportHistory(allImportJobs); }🤖 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 731 - 745, Both the loadLeads() and loadImportHistory() functions silently return when the fetch response is not OK, leaving loading spinners visible indefinitely with no error feedback to the user. Instead of silently returning on failed responses, add proper error handling to both functions that updates the UI to display an error state or message. This ensures users are informed when data loading fails rather than being left with an unresponsive loading state.
501-507:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape API/user-provided values before injecting HTML.
Line 504–506 and Line 576 interpolate unescaped values into
innerHTML(entry.email,entry.error,job.filename). That is a stored-XSS path if CSV/input payloads are reflected by the backend.Suggested fix
function renderImportErrorRows(job) { const tbody = document.getElementById('import-errors-body'); const errors = Array.isArray(job?.error_log) ? job.error_log : []; @@ tbody.innerHTML = errors.map(entry => ` <tr> - <td class="fw-semibold">${entry.row ?? '-'}</td> - <td>${entry.email || '<span class="text-muted">-</span>'}</td> - <td>${entry.error || '<span class="text-muted">-</span>'}</td> + <td class="fw-semibold">${escapeHtml(entry.row ?? '-')}</td> + <td>${entry.email ? escapeHtml(entry.email) : '<span class="text-muted">-</span>'}</td> + <td>${entry.error ? escapeHtml(entry.error) : '<span class="text-muted">-</span>'}</td> </tr> `).join(''); } @@ tbody.innerHTML = jobs.map(job => ` <tr> - <td class="fw-semibold">${job.filename}</td> + <td class="fw-semibold">${escapeHtml(job.filename)}</td> <td>${job.total_rows ?? 0}</td>Also applies to: 574-577
🤖 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 501 - 507, The code is vulnerable to stored-XSS attacks by injecting unescaped user-provided values directly into innerHTML. Create a utility function to escape HTML special characters (converting &, <, >, ", and ' to their HTML entity equivalents), then apply this escaping function to all user-controlled values before injecting them: entry.row, entry.email, and entry.error in the first location, and job.filename in the second location. Replace the direct template literal interpolation of these variables with calls to the HTML-escaping function to prevent malicious content from being executed as JavaScript.
🤖 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 `@frontend/leads.html`:
- Around line 510-512: The ID comparison in the openImportErrors function at
line 511 uses strict equality (===) to match item.id against jobId, but these
values may have different types (one could be a string while the other is
numeric), causing the lookup to fail intermittently. Convert both the item.id
and jobId to the same type before comparison, such as by converting both to
strings using String() or by converting both to numbers. Apply this same fix to
the second occurrence of this pattern at lines 588-590, ensuring consistent type
conversion is used in both locations for reliable ID matching.
---
Outside diff comments:
In `@frontend/leads.html`:
- Around line 731-745: Both the loadLeads() and loadImportHistory() functions
silently return when the fetch response is not OK, leaving loading spinners
visible indefinitely with no error feedback to the user. Instead of silently
returning on failed responses, add proper error handling to both functions that
updates the UI to display an error state or message. This ensures users are
informed when data loading fails rather than being left with an unresponsive
loading state.
- Around line 501-507: The code is vulnerable to stored-XSS attacks by injecting
unescaped user-provided values directly into innerHTML. Create a utility
function to escape HTML special characters (converting &, <, >, ", and ' to
their HTML entity equivalents), then apply this escaping function to all
user-controlled values before injecting them: entry.row, entry.email, and
entry.error in the first location, and job.filename in the second location.
Replace the direct template literal interpolation of these variables with calls
to the HTML-escaping function to prevent malicious content from being executed
as JavaScript.
🪄 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
510
to
512
| function openImportErrors(jobId) { | ||
| const job = allImportJobs.find(item => item.id === jobId); | ||
| if (!job) return; |
There was a problem hiding this comment.
Use type-stable ID matching for import job lookup.
Line 511 compares item.id === jobId; dataset.jobId is a string, while API IDs are often numeric. That can make the “View” button no-op intermittently.
Suggested fix
function openImportErrors(jobId) {
- const job = allImportJobs.find(item => item.id === jobId);
+ const job = allImportJobs.find(item => String(item.id) === String(jobId));
if (!job) return;Also applies to: 588-590
🤖 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 510 - 512, The ID comparison in the
openImportErrors function at line 511 uses strict equality (===) to match
item.id against jobId, but these values may have different types (one could be a
string while the other is numeric), causing the lookup to fail intermittently.
Convert both the item.id and jobId to the same type before comparison, such as
by converting both to strings using String() or by converting both to numbers.
Apply this same fix to the second occurrence of this pattern at lines 588-590,
ensuring consistent type conversion is used in both locations for reliable ID
matching.
4 tasks
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
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.
Related Issue
Closes #278
Summary of Changes
Resolved all unresolved Git merge conflict markers in
frontend/leads.html.While resolving the conflicts, I reviewed both versions of the code and retained the necessary functionality from each side to ensure no features were unintentionally removed.
Changes Made
<<<<<<<,=======,>>>>>>>)Type of Change
Testing
Steps performed:
Note: Features that depend on backend APIs could not be fully verified without a running backend service.
📸 Screenshots (if applicable)
N/A
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes