feat: add search autocomplete dropdown for mobile navbar#1010
feat: add search autocomplete dropdown for mobile navbar#1010ayesha1145 wants to merge 1 commit intoalphaonelabs:mainfrom
Conversation
👀 Peer Review RequiredHi @ayesha1145! This pull request does not yet have a peer review. Before this PR can be merged, please request a review from one of your peers:
Thank you for contributing! 🎉 |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds mobile search autocomplete to the base template: a mobile input and hidden dropdown plus client-side JavaScript that debounces input, cancels in-flight requests, fetches suggestions from Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as Client (JavaScript)
participant Server
User->>Browser: Type in mobile search input
Note over Browser: Debounce (250ms)
Browser->>Browser: If query length >= 2
alt valid query
Browser->>Server: GET /search_autocomplete?q=...
alt success
Server-->>Browser: suggestions[]
Browser->>Browser: Render dropdown (icon, title, teacher)
Browser->>Browser: Show dropdown
else failure or empty
Browser->>Browser: Clear/hide dropdown
end
else short query
Browser->>Browser: Hide/ignore dropdown
end
User->>Browser: Click suggestion / press Escape / click outside
Browser->>Browser: Navigate or close dropdown
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ayesha1145
force-pushed
the
feat/search-autocomplete-mobile
branch
from
817618b to
192d6a9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/templates/base.html (1)
547-557:⚠️ Potential issue | 🟠 MajorAlign this with a real combobox pattern.
role="listbox"on Line 557 is not enough here: the input never exposesaria-controls/aria-expanded, and the popup later renders anchors asrole="option"on Lines 1155-1159. That mixes link and listbox semantics, so assistive tech will not get a reliable autocomplete model. Either implement the full combobox pattern end-to-end, or treat this as a plain list of links and drop the listbox/option roles; if you keep links, add visiblefocus:/focus-visible:styles to each row.As per coding guidelines, "Ensure proper HTML structure and accessibility in templates", "Include proper ARIA labels where needed for accessibility", and "Add hover/focus states for interactive elements".
Also applies to: 1155-1159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/base.html` around lines 547 - 557, The autocomplete dropdown currently uses role="listbox" on the element with id="autocomplete-dropdown-mobile" while the input id="search-input-mobile" does not expose aria-controls/aria-expanded and the suggestion anchors are rendered with role="option", which mixes semantics; either convert this to a full ARIA combobox pattern by adding aria-haspopup="listbox", aria-expanded and aria-controls on search-input-mobile, manage aria-activedescendant on the input and ensure each suggestion has an id (not role="option" on anchors) and keyboard handling in the JS, or remove role="listbox"/role="option" entirely and treat suggestions as plain links styled for keyboard focus (add visible focus:/focus-visible: and hover styles on the suggestion anchors). Update the template around autocomplete-dropdown-mobile and the suggestion anchor generation (the current anchors at the later block) to follow one of these two options consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templates/base.html`:
- Line 1145: The template uses a reverse for the URL name search_autocomplete
which does not exist (only course_search is registered), causing NoReverseMatch
when rendering base.html; either add a URL pattern in your URLconf with
name="search_autocomplete" pointing to the existing autocomplete view (the same
view/function/class that implements course_search/autocomplete) or update the
template call to use the existing URL name course_search (and confirm the view
accepts the q parameter used by fetch). Ensure the chosen resolution keeps the
view name and query parameter behavior consistent across the template
(search_autocomplete reference) and the URLconf.
- Around line 1137-1176: When the input becomes shorter than 2 characters, abort
any in-flight request and cancel the debounce so stale fetches cannot repopulate
the dropdown: inside the query.length < 2 branch, call currentController.abort()
if currentController exists, clearTimeout(debounceTimer) (and set debounceTimer
= null if you track it), then clear/reset currentController, hide/clear dropdown
as already done; this ensures any pending fetch created in the debounce/fetch
block (using currentController and debounceTimer) is invalidated before
returning.
---
Outside diff comments:
In `@web/templates/base.html`:
- Around line 547-557: The autocomplete dropdown currently uses role="listbox"
on the element with id="autocomplete-dropdown-mobile" while the input
id="search-input-mobile" does not expose aria-controls/aria-expanded and the
suggestion anchors are rendered with role="option", which mixes semantics;
either convert this to a full ARIA combobox pattern by adding
aria-haspopup="listbox", aria-expanded and aria-controls on search-input-mobile,
manage aria-activedescendant on the input and ensure each suggestion has an id
(not role="option" on anchors) and keyboard handling in the JS, or remove
role="listbox"/role="option" entirely and treat suggestions as plain links
styled for keyboard focus (add visible focus:/focus-visible: and hover styles on
the suggestion anchors). Update the template around autocomplete-dropdown-mobile
and the suggestion anchor generation (the current anchors at the later block) to
follow one of these two options consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c19e6393-6fcc-4faf-9abc-0649b3e87cb2
📒 Files selected for processing (1)
web/templates/base.html
ayesha1145
force-pushed
the
feat/search-autocomplete-mobile
branch
from
192d6a9 to
10dada3
Compare
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 (1)
web/templates/base.html (1)
546-557:⚠️ Potential issue | 🟡 MinorAdd ARIA attributes to the input for complete accessibility.
The dropdown has
role="listbox"but the input lacksaria-controlsandaria-expandedattributes. Screen readers won't announce the relationship between the input and the suggestions dropdown.♿ Proposed fix for accessibility
<input type="text" id="search-input-mobile" autocomplete="off" + aria-controls="autocomplete-dropdown-mobile" + aria-expanded="false" + aria-autocomplete="list" name="q"Then update the JavaScript to toggle
aria-expanded:// When showing dropdown: input.setAttribute('aria-expanded', 'true'); // When hiding dropdown: input.setAttribute('aria-expanded', 'false');As per coding guidelines: "Include proper ARIA labels where needed for accessibility."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/base.html` around lines 546 - 557, Add ARIA linkage and state to the mobile search input: update the input element with id "search-input-mobile" to include aria-controls="autocomplete-dropdown-mobile" and an initial aria-expanded="false" (and aria-haspopup="listbox" if desired), and then update the JS that shows/hides the element with id "autocomplete-dropdown-mobile" to set document.getElementById('search-input-mobile').setAttribute('aria-expanded','true') when showing and setAttribute('aria-expanded','false') when hiding so screen readers can discover the relationship and current state.
♻️ Duplicate comments (2)
web/templates/base.html (2)
1145-1145:⚠️ Potential issue | 🔴 CriticalThe URL name
search_autocompletedoes not exist and will break template rendering.The
{% url 'search_autocomplete' %}tag will raiseNoReverseMatchbecause this URL is not defined inweb/urls.py. Sincebase.htmlis extended by every page, this will break the entire site.Either add the URL pattern in
urls.py:path("courses/search/autocomplete/", views.search_autocomplete, name="search_autocomplete"),Or if an existing endpoint returns JSON with the expected structure
{"results": [{"title": "...", "teacher": "...", "url": "..."}]}, update the template to use that URL name instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/base.html` at line 1145, The template uses the non-existent URL name 'search_autocomplete' in base.html (fetch("{% url 'search_autocomplete' %}...")), causing NoReverseMatch; either add a URL pattern that names the endpoint 'search_autocomplete' pointing to the view function (e.g., views.search_autocomplete) in your URL conf, or change the template to reference an existing URL name that returns the expected JSON shape ({"results":[...]})—update the path name in urls.py or the string in the {% url %} call accordingly so the template resolves at render time.
1137-1176:⚠️ Potential issue | 🟠 MajorAbort in-flight requests and guard against stale responses when query becomes too short.
When input drops below 2 characters (Lines 1137-1140), the pending fetch continues running. If it resolves later, it will repopulate the dropdown with stale suggestions even though the input is now empty or too short. Additionally, the catch block should ignore
AbortErrorto avoid clearing the dropdown unnecessarily when requests are intentionally cancelled.🔧 Proposed fix
if (query.length < 2) { + if (currentController) { + currentController.abort(); + currentController = null; + } dropdown.classList.add('hidden'); - dropdown.innerHTML = ''; + dropdown.replaceChildren(); return; } debounceTimer = setTimeout(() => { if (currentController) currentController.abort(); currentController = new AbortController(); + const requestQuery = query; fetch("{% url 'search_autocomplete' %}" + '?q=' + encodeURIComponent(query), { signal: currentController.signal }) .then(r => r.json()) .then(data => { + // Guard against stale response + if (input.value.trim() !== requestQuery) return; if (!data.results || data.results.length === 0) { dropdown.classList.add('hidden'); - dropdown.innerHTML = ''; + dropdown.replaceChildren(); return; } // ... rest unchanged }) - .catch(() => { + .catch((error) => { + if (error.name === 'AbortError') return; dropdown.classList.add('hidden'); dropdown.replaceChildren(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/base.html` around lines 1137 - 1176, When query.length < 2, ensure any in-flight request is aborted and its response won't repopulate the dropdown: call currentController.abort() (if set), clear debounceTimer, set currentController = null, and then hide/clear the dropdown as you already do. In the fetch promise chain, update the catch handler to detect and ignore AbortError (e.g., if (err.name === 'AbortError') return) so intentional cancellations don't clear the dropdown, and only perform dropdown.classList.add('hidden') / dropdown.replaceChildren() for non-abort errors; keep references to debounceTimer and currentController to locate the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templates/base.html`:
- Around line 1186-1188: Add keyboard navigation to the existing input keydown
handler so users can traverse and select dropdown items: introduce an
activeIndex variable (initialized -1) and on 'ArrowDown'/'ArrowUp' update
activeIndex within bounds of dropdown.querySelectorAll('a[role="option"]')
(named items), call an updateActiveItem(items) helper to set aria-selected and
the focused CSS class, handle 'Enter' to trigger items[activeIndex].click(), and
reset activeIndex and hide the dropdown on 'Escape'; modify the current
input.addEventListener('keydown', ...) logic to incorporate these branches and
call updateActiveItem when activeIndex changes.
---
Outside diff comments:
In `@web/templates/base.html`:
- Around line 546-557: Add ARIA linkage and state to the mobile search input:
update the input element with id "search-input-mobile" to include
aria-controls="autocomplete-dropdown-mobile" and an initial
aria-expanded="false" (and aria-haspopup="listbox" if desired), and then update
the JS that shows/hides the element with id "autocomplete-dropdown-mobile" to
set
document.getElementById('search-input-mobile').setAttribute('aria-expanded','true')
when showing and setAttribute('aria-expanded','false') when hiding so screen
readers can discover the relationship and current state.
---
Duplicate comments:
In `@web/templates/base.html`:
- Line 1145: The template uses the non-existent URL name 'search_autocomplete'
in base.html (fetch("{% url 'search_autocomplete' %}...")), causing
NoReverseMatch; either add a URL pattern that names the endpoint
'search_autocomplete' pointing to the view function (e.g.,
views.search_autocomplete) in your URL conf, or change the template to reference
an existing URL name that returns the expected JSON shape
({"results":[...]})—update the path name in urls.py or the string in the {% url
%} call accordingly so the template resolves at render time.
- Around line 1137-1176: When query.length < 2, ensure any in-flight request is
aborted and its response won't repopulate the dropdown: call
currentController.abort() (if set), clear debounceTimer, set currentController =
null, and then hide/clear the dropdown as you already do. In the fetch promise
chain, update the catch handler to detect and ignore AbortError (e.g., if
(err.name === 'AbortError') return) so intentional cancellations don't clear the
dropdown, and only perform dropdown.classList.add('hidden') /
dropdown.replaceChildren() for non-abort errors; keep references to
debounceTimer and currentController to locate the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74a49421-1346-4e4a-9512-cf0772c32008
📒 Files selected for processing (1)
web/templates/base.html
💬 Unresolved Review ConversationsHi @ayesha1145! 👋 This pull request currently has 3 unresolved review conversations. Please address all review feedback and push a new commit to resolve them before this PR can be merged. Steps to resolve:
Once all conversations are resolved, this notice will be removed automatically. Thank you! 🙏 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Addressed CodeRabbit comments:
|
1 participant
Extends the search autocomplete feature to the mobile navigation menu.
web/templates/base.html: addedidandautocomplete="off"to mobile search input, dropdown container inside relative parent, debounced fetch with AbortController, safe DOM construction, ARIA roles, Escape/outside-click dismissalReuses the existing
search_autocompleteendpoint — no backend changes needed.Summary by CodeRabbit