Fix anchor tag URL navigation issue and improve link handling#252
Fix anchor tag URL navigation issue and improve link handling#252DHRUVI5674 wants to merge 1 commit into
Conversation
|
@DHRUVI5674 is attempting to deploy a commit to the xthxr's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughHash-based ChangesNavigation link cleanup and client-side scroll routing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hi @xthxr , |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
public/landing.html (2)
396-401: 💤 Low valueAdd console warning for missing section IDs.
When a user navigates to a URL with a non-existent section ID (e.g.,
/invalid-section), the function silently fails. Adding a console warning would aid debugging and alert developers to broken links or bookmarks.📝 Suggested enhancement
function scrollToSection(sectionId) { const element = document.getElementById(sectionId); if (element) { element.scrollIntoView({ behavior: 'smooth' }); + } else { + console.warn(`Section not found: ${sectionId}`); } }🤖 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 `@public/landing.html` around lines 396 - 401, The scrollToSection function currently fails silently when the requested section ID does not exist on the page. Add an else clause after the element existence check to log a console warning that includes the sectionId parameter when the element is not found. This will help developers identify broken links or incorrect bookmark URLs during debugging.
316-316: 💤 Low valueConsider replacing non-navigating anchor with a semantic element.
The Discord link uses
javascript:void(0)to prevent navigation, but per the PR objectives (Issue#240), non-navigating anchors should be replaced with appropriate semantic elements like<button>or<span>. If this link will eventually navigate to Discord, keep the anchor but update the href when ready. If it's meant to trigger an action (e.g., open a modal), use a<button>.📋 Example alternatives
If the link will eventually navigate:
<!-- Keep as anchor, mark as coming soon --> <li><a href="https://discord.gg/yourserver" class="hover:text-accent transition-colors" target="_blank" rel="noopener">Discord</a></li>If it's a placeholder with no action:
<li><span class="text-gray-400 cursor-not-allowed">Discord (Coming Soon)</span></li>If it should trigger an action:
<li><button type="button" class="hover:text-accent transition-colors text-left" onclick="alert('Discord coming soon')">Discord</button></li>🤖 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 `@public/landing.html` at line 316, The Discord link at line 316 uses javascript:void(0) which is not semantic and should be replaced per Issue `#240`. Determine the intended functionality: if it will eventually navigate to Discord, replace the href with the actual Discord URL (e.g., https://discord.gg/yourserver); if it's a placeholder with no action, replace the anchor element with a span element and add "Coming Soon" text; if it should trigger an action, replace the anchor with a button element instead. Update the element and any necessary styling classes to match the selected approach.
🤖 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 `@public/index.html`:
- Around line 128-130: The Profile and Settings dropdown links in
public/index.html have been changed to use javascript:void(0); without
implementing proper click handlers or navigation attributes, making them
non-functional. At public/index.html lines 128-130, add a data-page="profile"
attribute to the Profile link anchor element to enable navigation through the
existing navigation system that intercepts clicks on elements with dataset.page
(as referenced in app.js:209-252). Similarly, at public/index.html lines
131-133, add a data-page="settings" attribute to the Settings link anchor
element. Alternatively, you may replace these anchor elements with functional
button elements that include the appropriate data-page attributes and are
properly styled to match the dropdown interface.
- Around line 68-71: Replace the non-semantic `<a>` tag with
`href="javascript:void(0);"` with a `<button>` element since the reportBugBtn
element triggers a modal dialog via JavaScript and does not navigate. Preserve
the id attribute (reportBugBtn) and classes (nav-item report-bug-btn) on the new
button element, keeping the icon and span text structure intact. Then verify
that the CSS styles for button.nav-item match the styling previously applied to
a.nav-item to ensure consistent visual appearance.
- Around line 135-137: Replace the anchor element with the logoutBtn id in
public/index.html with a semantic button element instead. Change the structure
from an anchor tag with javascript:void(0) to a button tag while preserving the
id="logoutBtn", the dropdown-item class, and the icon markup. Verify that the
CSS styling for the .dropdown-item class works correctly with button elements,
or update the CSS if necessary to ensure the button displays and behaves as
intended with the same visual appearance as the original anchor link.
In `@public/landing.html`:
- Around line 41-44: Remove the redundant inline onclick handlers that are
causing duplicate scrollToSection calls. In public/landing.html lines 41-44
(navbar links for How it Works, Features, Testimonials, and FAQ), remove the
onclick="scrollToSection(...)" attribute from each anchor tag. In
public/landing.html lines 307-309 (footer Product links for Features, How it
Works, and FAQ), also remove the onclick="scrollToSection(...)" attribute from
each anchor tag. The DOMContentLoaded event listener already handles scroll
navigation for all links with href attributes starting with "/", so these inline
handlers are redundant and cause double-triggering of the scroll functionality.
- Around line 403-415: The selector a[href^="/"] in the DOMContentLoaded event
listener is matching the logo link (href="/") which blocks navigation without
performing any action. Fix this by refining the selector to exclude root
navigation (e.g., a[href^="/"][href!="/"]) or by adding an explicit check within
the click event handler to allow root path navigation to proceed with default
behavior. The issue affects the DOMContentLoaded listener and the nested click
event handler on link elements, so ensure any fix allows the logo and root
navigation to work normally while still intercepting section navigation.
---
Nitpick comments:
In `@public/landing.html`:
- Around line 396-401: The scrollToSection function currently fails silently
when the requested section ID does not exist on the page. Add an else clause
after the element existence check to log a console warning that includes the
sectionId parameter when the element is not found. This will help developers
identify broken links or incorrect bookmark URLs during debugging.
- Line 316: The Discord link at line 316 uses javascript:void(0) which is not
semantic and should be replaced per Issue `#240`. Determine the intended
functionality: if it will eventually navigate to Discord, replace the href with
the actual Discord URL (e.g., https://discord.gg/yourserver); if it's a
placeholder with no action, replace the anchor element with a span element and
add "Coming Soon" text; if it should trigger an action, replace the anchor with
a button element instead. Update the element and any necessary styling classes
to match the selected approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4aff72c6-a448-4857-888d-c1a7a537acf6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
public/index.htmlpublic/landing.html
| <a href="javascript:void(0);" class="nav-item report-bug-btn" id="reportBugBtn"> | ||
| <i class="fas fa-bug"></i> | ||
| <span>Report Bug</span> | ||
| </a> |
There was a problem hiding this comment.
Inconsistent with PR objectives: should use semantic HTML.
The PR objectives specify replacing non-navigating anchors with semantic elements like <button>, but this change keeps the <a> tag and only updates the href. Additionally, javascript:void(0) is generally considered bad practice.
Since this element triggers a modal via JavaScript (confirmed in snippet 1 from public/js/app.js:485-491) and does not navigate, it should be a <button> element.
♻️ Proposed refactor to semantic HTML
- <a href="javascript:void(0);" class="nav-item report-bug-btn" id="reportBugBtn">
+ <button type="button" class="nav-item report-bug-btn" id="reportBugBtn">
<i class="fas fa-bug"></i>
<span>Report Bug</span>
- </a>
+ </button>CSS may need adjustment to style button.nav-item identically to a.nav-item. Verify in css/styles.css.
📝 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.
| <a href="javascript:void(0);" class="nav-item report-bug-btn" id="reportBugBtn"> | |
| <i class="fas fa-bug"></i> | |
| <span>Report Bug</span> | |
| </a> | |
| <button type="button" class="nav-item report-bug-btn" id="reportBugBtn"> | |
| <i class="fas fa-bug"></i> | |
| <span>Report Bug</span> | |
| </button> |
🤖 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 `@public/index.html` around lines 68 - 71, Replace the non-semantic `<a>` tag
with `href="javascript:void(0);"` with a `<button>` element since the
reportBugBtn element triggers a modal dialog via JavaScript and does not
navigate. Preserve the id attribute (reportBugBtn) and classes (nav-item
report-bug-btn) on the new button element, keeping the icon and span text
structure intact. Then verify that the CSS styles for button.nav-item match the
styling previously applied to a.nav-item to ensure consistent visual appearance.
| <a href="javascript:void(0);" class="dropdown-item"> | ||
| <i class="fas fa-user"></i> Profile | ||
| </a> |
There was a problem hiding this comment.
Profile and Settings dropdown items appear non-functional. Both links changed from hash-based anchors (#profile, #settings) to javascript:void(0); without adding click handlers or data-page attributes. According to snippet 2 (public/js/app.js:209-252), the navigation system only intercepts clicks on elements with dataset.page. Without handlers, these links will not respond to user interaction.
public/index.html#L128-L130: Add a click handler ordata-page="profile"attribute to the Profile link, or replace with a functional<button>element that navigates to the profile page.public/index.html#L131-L133: Add a click handler ordata-page="profile"attribute to the Settings link, or replace with a functional<button>element that navigates to the settings page.
📍 Affects 1 file
public/index.html#L128-L130(this comment)public/index.html#L131-L133
🤖 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 `@public/index.html` around lines 128 - 130, The Profile and Settings dropdown
links in public/index.html have been changed to use javascript:void(0); without
implementing proper click handlers or navigation attributes, making them
non-functional. At public/index.html lines 128-130, add a data-page="profile"
attribute to the Profile link anchor element to enable navigation through the
existing navigation system that intercepts clicks on elements with dataset.page
(as referenced in app.js:209-252). Similarly, at public/index.html lines
131-133, add a data-page="settings" attribute to the Settings link anchor
element. Alternatively, you may replace these anchor elements with functional
button elements that include the appropriate data-page attributes and are
properly styled to match the dropdown interface.
| <a href="javascript:void(0);" class="dropdown-item" id="logoutBtn"> | ||
| <i class="fas fa-sign-out-alt"></i> Logout | ||
| </a> |
There was a problem hiding this comment.
Inconsistent with PR objectives: should use semantic HTML.
Like the Report Bug link, this Logout anchor should be a <button> element per the PR objectives. Although the click handler exists (confirmed in snippet 3 from public/js/app.js:843-858), using javascript:void(0) in an anchor for a non-navigating action is not semantic HTML.
♻️ Proposed refactor to semantic HTML
- <a href="javascript:void(0);" class="dropdown-item" id="logoutBtn">
+ <button type="button" class="dropdown-item" id="logoutBtn">
<i class="fas fa-sign-out-alt"></i> Logout
- </a>
+ </button>Verify that CSS for .dropdown-item handles both <a> and <button> elements, or update styles accordingly.
📝 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.
| <a href="javascript:void(0);" class="dropdown-item" id="logoutBtn"> | |
| <i class="fas fa-sign-out-alt"></i> Logout | |
| </a> | |
| <button type="button" class="dropdown-item" id="logoutBtn"> | |
| <i class="fas fa-sign-out-alt"></i> Logout | |
| </button> |
🤖 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 `@public/index.html` around lines 135 - 137, Replace the anchor element with
the logoutBtn id in public/index.html with a semantic button element instead.
Change the structure from an anchor tag with javascript:void(0) to a button tag
while preserving the id="logoutBtn", the dropdown-item class, and the icon
markup. Verify that the CSS styling for the .dropdown-item class works correctly
with button elements, or update the CSS if necessary to ensure the button
displays and behaves as intended with the same visual appearance as the original
anchor link.
| <a href="/how-it-works" class="text-sm font-medium text-gray-600 dark:text-gray-400 hover:text-accent dark:hover:text-white transition-colors uppercase tracking-wider" onclick="scrollToSection('how-it-works')">How it Works</a> | ||
| <a href="/features" class="text-sm font-medium text-gray-600 dark:text-gray-400 hover:text-accent dark:hover:text-white transition-colors uppercase tracking-wider" onclick="scrollToSection('features')">Features</a> | ||
| <a href="/testimonials" class="text-sm font-medium text-gray-600 dark:text-gray-400 hover:text-accent dark:hover:text-white transition-colors uppercase tracking-wider" onclick="scrollToSection('testimonials')">Testimonials</a> | ||
| <a href="/faq" class="text-sm font-medium text-gray-600 dark:text-gray-400 hover:text-accent dark:hover:text-white transition-colors uppercase tracking-wider" onclick="scrollToSection('faq')">FAQ</a> |
There was a problem hiding this comment.
Remove all redundant inline onclick handlers (navbar and footer).
Both the navbar and footer links have inline onclick="scrollToSection(...)" attributes, but the DOMContentLoaded block at line 405 attaches click listeners to all links with href^="/", including these same links. Every click triggers scrollToSection twice: once inline, once from the attached listener, causing redundant scroll operations.
public/landing.html#L41-L44: Removeonclickfrom navbar links (How it Works, Features, Testimonials, FAQ).public/landing.html#L307-L309: Removeonclickfrom footer Product links (Features, How it Works, FAQ).
📍 Affects 1 file
public/landing.html#L41-L44(this comment)public/landing.html#L307-L309
🤖 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 `@public/landing.html` around lines 41 - 44, Remove the redundant inline
onclick handlers that are causing duplicate scrollToSection calls. In
public/landing.html lines 41-44 (navbar links for How it Works, Features,
Testimonials, and FAQ), remove the onclick="scrollToSection(...)" attribute from
each anchor tag. In public/landing.html lines 307-309 (footer Product links for
Features, How it Works, and FAQ), also remove the onclick="scrollToSection(...)"
attribute from each anchor tag. The DOMContentLoaded event listener already
handles scroll navigation for all links with href attributes starting with "/",
so these inline handlers are redundant and cause double-triggering of the scroll
functionality.
| document.addEventListener('DOMContentLoaded', () => { | ||
| // Attach click handlers to navigation links that start with '/' | ||
| document.querySelectorAll('a[href^="/"]').forEach(link => { | ||
| link.addEventListener('click', (e) => { | ||
| e.preventDefault(); | ||
| const href = link.getAttribute('href'); | ||
| const section = href.startsWith('/') ? href.slice(1) : href; | ||
| // Update URL without reloading | ||
| history.pushState(null, '', href); | ||
| // Scroll to target section | ||
| scrollToSection(section); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Overly broad selector breaks navigation and blocks logo link.
The selector a[href^="/"] matches all links starting with /, including the logo (href="/" at line 38) and any potential links to other app pages. When the logo is clicked:
- Line 407 prevents default navigation
- Line 409 extracts section as
"/"→.slice(1)→"" - Line 411 pushes state to
/(no-op if already at root) - Line 413 calls
scrollToSection(""), which does nothing
The result: clicking the logo is blocked but doesn't scroll or navigate. Exclude the root path or handle it explicitly.
🔧 Proposed fix to exclude root navigation
document.addEventListener('DOMContentLoaded', () => {
// Attach click handlers to navigation links that start with '/'
- document.querySelectorAll('a[href^="/"]').forEach(link => {
+ document.querySelectorAll('a[href^="/"][href!="/"]').forEach(link => {
link.addEventListener('click', (e) => {
e.preventDefault();
const href = link.getAttribute('href');Alternatively, handle root explicitly inside the listener:
document.querySelectorAll('a[href^="/"]').forEach(link => {
link.addEventListener('click', (e) => {
+ const href = link.getAttribute('href');
+ if (href === '/') return; // Allow default navigation to root
e.preventDefault();
- const href = link.getAttribute('href');
const section = href.startsWith('/') ? href.slice(1) : href;📝 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.
| document.addEventListener('DOMContentLoaded', () => { | |
| // Attach click handlers to navigation links that start with '/' | |
| document.querySelectorAll('a[href^="/"]').forEach(link => { | |
| link.addEventListener('click', (e) => { | |
| e.preventDefault(); | |
| const href = link.getAttribute('href'); | |
| const section = href.startsWith('/') ? href.slice(1) : href; | |
| // Update URL without reloading | |
| history.pushState(null, '', href); | |
| // Scroll to target section | |
| scrollToSection(section); | |
| }); | |
| }); | |
| document.addEventListener('DOMContentLoaded', () => { | |
| // Attach click handlers to navigation links that start with '/' | |
| document.querySelectorAll('a[href^="/"][href!="/"]').forEach(link => { | |
| link.addEventListener('click', (e) => { | |
| e.preventDefault(); | |
| const href = link.getAttribute('href'); | |
| const section = href.startsWith('/') ? href.slice(1) : href; | |
| // Update URL without reloading | |
| history.pushState(null, '', href); | |
| // Scroll to target section | |
| scrollToSection(section); | |
| }); | |
| }); |
| document.addEventListener('DOMContentLoaded', () => { | |
| // Attach click handlers to navigation links that start with '/' | |
| document.querySelectorAll('a[href^="/"]').forEach(link => { | |
| link.addEventListener('click', (e) => { | |
| e.preventDefault(); | |
| const href = link.getAttribute('href'); | |
| const section = href.startsWith('/') ? href.slice(1) : href; | |
| // Update URL without reloading | |
| history.pushState(null, '', href); | |
| // Scroll to target section | |
| scrollToSection(section); | |
| }); | |
| }); | |
| document.addEventListener('DOMContentLoaded', () => { | |
| // Attach click handlers to navigation links that start with '/' | |
| document.querySelectorAll('a[href^="/"]').forEach(link => { | |
| link.addEventListener('click', (e) => { | |
| const href = link.getAttribute('href'); | |
| if (href === '/') return; // Allow default navigation to root | |
| e.preventDefault(); | |
| const section = href.startsWith('/') ? href.slice(1) : href; | |
| // Update URL without reloading | |
| history.pushState(null, '', href); | |
| // Scroll to target section | |
| scrollToSection(section); | |
| }); | |
| }); |
🤖 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 `@public/landing.html` around lines 403 - 415, The selector a[href^="/"] in the
DOMContentLoaded event listener is matching the logo link (href="/") which
blocks navigation without performing any action. Fix this by refining the
selector to exclude root navigation (e.g., a[href^="/"][href!="/"]) or by adding
an explicit check within the click event handler to allow root path navigation
to proceed with default behavior. The issue affects the DOMContentLoaded
listener and the nested click event handler on link elements, so ensure any fix
allows the logo and root navigation to work normally while still intercepting
section navigation.
1 participant
Related Issue
Fixes #240
Summary
This PR resolves an issue where anchor tags were not handling URLs correctly, resulting in incorrect navigation behavior.
Changes Made
Fixed URL handling in anchor tags.
Corrected link navigation behavior.
Added validation for URL generation.
Improved handling of external and internal links.
Updated related tests to cover the fix.
Testing
Verified navigation for internal links.
Verified navigation for external links.
Confirmed anchor tags redirect to the correct URLs.
Tested edge cases with invalid and empty URLs.
Expected Result
Users can now navigate correctly through anchor tags without broken or incorrect URL redirections.
Summary by CodeRabbit