fix: remove hardcoded aria-current from sidebar nav links#317
Conversation
- Removed hardcoded aria-current=page from all sidebar nav links - Dynamic active link detection already handled by setActiveNavLink() in main.js - campaign-builder.html has no sidebar, no change needed Fixes Kuldeeep18#62
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR removes the hardcoded ChangesDynamic navigation active state and leads.html restructuring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/leads.html (1)
243-344:⚠️ Potential issue | 🔴 CriticalCRITICAL: Unresolved git merge conflicts in file.
This file contains multiple unresolved git merge conflict blocks that must be resolved before merging:
- Lines 243–310: Conflict between HEAD (filter panel) and upstream/main (import history)
- Lines 442–495: Conflict in script state and utility functions
- Lines 506–540: Conflict in render functions (renderImportErrorRows, etc.)
- Lines 634–798: Conflict in filter panel and import history logic
- Lines 873–882: Conflict in CSV upload handler
All conflict markers (
<<<<<<<,=======,>>>>>>>) must be manually resolved. Decide which version(s) to keep for each block, then remove the markers.This is a blocker for merge. Cannot review code while conflicts remain unresolved.
🤖 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 243 - 344, The file contains multiple unresolved git merge conflict blocks marked with <<<<<<< HEAD, =======, and >>>>>>> upstream/main markers that must be resolved before merging. For each conflict block in the file (including the filter panel section, script state and utility functions, render functions like renderImportErrorRows, filter panel and import history logic, and the CSV upload handler), manually decide which version to keep by reviewing both the HEAD version and the upstream/main version, then remove all the conflict markers while retaining only the chosen code. Ensure no marker symbols remain in the final file.
🤖 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.
Outside diff comments:
In `@frontend/leads.html`:
- Around line 243-344: The file contains multiple unresolved git merge conflict
blocks marked with <<<<<<< HEAD, =======, and >>>>>>> upstream/main markers that
must be resolved before merging. For each conflict block in the file (including
the filter panel section, script state and utility functions, render functions
like renderImportErrorRows, filter panel and import history logic, and the CSV
upload handler), manually decide which version to keep by reviewing both the
HEAD version and the upstream/main version, then remove all the conflict markers
while retaining only the chosen code. Ensure no marker symbols remain in the
final file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b98c284-e24c-4dfb-a1b9-16b22370a77b
📒 Files selected for processing (5)
frontend/analytics.htmlfrontend/campaigns.htmlfrontend/dashboard.htmlfrontend/leads.htmlfrontend/settings.html
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/leads.html (4)
856-861:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the current filter/search state after upload refresh.
loadLeads()always fetches/leads/and renders all rows, so after a CSV upload the table can ignore active filters/search while the UI still shows them as active. Refresh the current view instead.🐛 Proposed fix
if (res.status === 202) { statusEl.innerHTML = '<span class="text-success"><i class="bi bi-check-circle me-1" aria-hidden="true"></i>File uploaded. Processing now...</span>'; - await loadLeads(); + if (hasActiveFilters(activeFilters)) { + await applyFilters(); + } else { + await loadLeads(); + } + applySearch(); await loadImportHistory(); fileInput.value = ''; bootstrap.Modal.getOrCreateInstance(document.getElementById('uploadModal')).hide();🤖 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 856 - 861, After a CSV file upload, the loadLeads() call refreshes the table with all rows, ignoring any active filters or search criteria that were previously applied. Instead of calling loadLeads() unconditionally after the upload, modify the refresh logic to preserve the current filter and search state by applying the same filter/search parameters that were active before the upload. This ensures the UI and table data remain synchronized after the CSV upload completes.
515-519:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape import history and row-error values before injecting HTML.
job.filename,entry.email, andentry.errorcan originate from uploaded CSV data/import metadata, but they are written directly throughinnerHTML. Escape them before rendering to avoid stored XSS in the import history/error modal.🛡️ Proposed fix
- tbody.innerHTML = errors.map(entry => ` + tbody.innerHTML = errors.map(entry => { + const row = escapeHtml(entry.row ?? '-'); + const email = entry.email + ? escapeHtml(entry.email) + : '<span class="text-muted">-</span>'; + const error = entry.error + ? escapeHtml(entry.error) + : '<span class="text-muted">-</span>'; + return ` <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">${row}</td> + <td>${email}</td> + <td>${error}</td> </tr> - `).join(''); + `; + }).join('');- tbody.innerHTML = jobs.map(job => ` + tbody.innerHTML = jobs.map(job => { + const jobId = escapeHtml(String(job.id ?? '')); + return ` <tr> - <td class="fw-semibold">${job.filename}</td> + <td class="fw-semibold">${escapeHtml(job.filename)}</td> <td>${job.total_rows ?? 0}</td> <td><span class="chip" style="background:`#dcfce7`;color:`#166534`;">${job.imported_count ?? 0}</span></td> <td><span class="chip" style="background:`#fee2e2`;color:`#b91c1c`;">${job.failed_count ?? 0}</span></td> <td>${formatTimestamp(job.created_at)}</td> <td> ${ (job.failed_count || 0) > 0 - ? `<button class="btn btn-sm btn-outline-danger view-import-errors-btn" data-job-id="${job.id}"><i class="bi bi-exclamation-triangle me-1" aria-hidden="true"></i>View</button>` + ? `<button class="btn btn-sm btn-outline-danger view-import-errors-btn" data-job-id="${jobId}"><i class="bi bi-exclamation-triangle me-1" aria-hidden="true"></i>View</button>` : '<span class="text-muted small">None</span>' } </td> </tr> - `).join(''); + `; + }).join('');Also applies to: 593-603
🤖 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 515 - 519, The code is vulnerable to stored XSS attacks because user-supplied values from CSV uploads (entry.email, entry.error, and job.filename) are directly injected into HTML through innerHTML without escaping. Apply proper HTML escaping to all user-provided values before inserting them into the template literal string. This should be done in the tbody.innerHTML template where entry.email and entry.error are interpolated, as well as in the similar code block around lines 593-603 where job.filename is used. Create a helper function to escape HTML entities (or use a library) and apply it to these values so that any HTML special characters are properly encoded.
524-525:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize import job IDs before lookup.
btn.dataset.jobIdis a string, so numeric API IDs will not matchitem.id === jobId; the “View” button can silently do nothing for failed imports.🐛 Proposed fix
- const job = allImportJobs.find(item => item.id === jobId); + const job = allImportJobs.find(item => String(item.id) === String(jobId));Also applies to: 610-611
🤖 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 524 - 525, The openImportErrors function receives jobId as a string from btn.dataset.jobId (HTML data attributes are always strings), but item.id from the API is likely a number, causing the strict equality check to fail silently. Normalize the jobId to a number using parseInt or Number conversion before comparing with item.id in the find method. Apply the same fix to the related code mentioned at lines 610-611 to ensure consistent type handling across all import job ID lookups.
155-155:⚠️ Potential issue | 🔴 CriticalAdd dynamic
aria-current="page"management tosetActiveNavLink().Removing the hardcoded
aria-current="page"attribute is a critical accessibility issue. ThesetActiveNavLink()function inmain.js(lines 172–200) currently manages only the visual.activeCSS class but does not setaria-current="page"on the active link or remove it from inactive links. Screen readers rely on this attribute to announce the current page; the visual state alone is insufficient. UpdatesetActiveNavLink()to setaria-current="page"on the matched active link and remove the attribute from all other nav-links.🤖 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 155, The setActiveNavLink() function in main.js (lines 172-200) currently manages only the visual .active CSS class for navigation links but does not manage the aria-current="page" attribute required for screen reader accessibility. Update the setActiveNavLink() function to remove the aria-current="page" attribute from all nav-links before adding it to the matched active link, ensuring that only the current page link has this attribute set. This will properly communicate the active page state to assistive technologies.
🤖 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.
Outside diff comments:
In `@frontend/leads.html`:
- Around line 856-861: After a CSV file upload, the loadLeads() call refreshes
the table with all rows, ignoring any active filters or search criteria that
were previously applied. Instead of calling loadLeads() unconditionally after
the upload, modify the refresh logic to preserve the current filter and search
state by applying the same filter/search parameters that were active before the
upload. This ensures the UI and table data remain synchronized after the CSV
upload completes.
- Around line 515-519: The code is vulnerable to stored XSS attacks because
user-supplied values from CSV uploads (entry.email, entry.error, and
job.filename) are directly injected into HTML through innerHTML without
escaping. Apply proper HTML escaping to all user-provided values before
inserting them into the template literal string. This should be done in the
tbody.innerHTML template where entry.email and entry.error are interpolated, as
well as in the similar code block around lines 593-603 where job.filename is
used. Create a helper function to escape HTML entities (or use a library) and
apply it to these values so that any HTML special characters are properly
encoded.
- Around line 524-525: The openImportErrors function receives jobId as a string
from btn.dataset.jobId (HTML data attributes are always strings), but item.id
from the API is likely a number, causing the strict equality check to fail
silently. Normalize the jobId to a number using parseInt or Number conversion
before comparing with item.id in the find method. Apply the same fix to the
related code mentioned at lines 610-611 to ensure consistent type handling
across all import job ID lookups.
- Line 155: The setActiveNavLink() function in main.js (lines 172-200) currently
manages only the visual .active CSS class for navigation links but does not
manage the aria-current="page" attribute required for screen reader
accessibility. Update the setActiveNavLink() function to remove the
aria-current="page" attribute from all nav-links before adding it to the matched
active link, ensuring that only the current page link has this attribute set.
This will properly communicate the active page state to assistive technologies.
|
All changes for this PR are now complete. Here's a summary of what was done: Removed hardcoded aria-current="page" from sidebar nav links across dashboard.html, leads.html, campaigns.html, analytics.html, and settings.html The only remaining warning is the duplicate CSS in campaigns.html (~350 lines of inline CSS that should reference theme.css instead). This is a separate concern from the aria-current fix and was pre-existing before this PR. I will open a dedicated issue to track and clean that up properly so it gets the attention it deserves without blocking this merge. |
2 participants
Fixes #62
Changes made
aria-current="page"from sidebar nav links in dashboard.html, leads.html, campaigns.html, analytics.html, settings.htmlTesting
Verified all pages show correct active sidebar link. Campaigns highlighted on campaign-builder.html. No visual regressions.
Summary by CodeRabbit