Conversation
📝 WalkthroughWalkthroughAdds a typeahead autocomplete: new DRF serializers and an Changes
Sequence DiagramsequenceDiagram
autonumber
participant User
participant Browser
participant API as /api/autocomplete/
participant Views as views.search
participant DB as Database/Index
participant UI as Suggestions DOM
User->>Browser: type query
Browser->>Browser: debounce 250ms, abort previous via AbortController
Browser->>API: GET /api/autocomplete/?q=...&mode=...
API->>Views: call autocomplete view
Views->>DB: query courses, instructors, clubs (with limits)
DB-->>Views: matched results
Views-->>API: serialized JSON (courses, instructors, clubs)
API-->>Browser: JSON response
Browser->>UI: render grouped suggestions
User->>UI: click suggestion
UI->>Browser: set input, clear suggestions, submit form
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tcf_website/views/search.py (2)
348-396: Renamesimilarity_secondtosimilarity_lastfor consistency.Line 360 uses
similarity_secondfor thelast_namefield, but the existingfetch_instructorsfunction (line 215) usessimilarity_lastfor the same purpose. This inconsistency reduces code readability.Apply this diff:
instructors = ( Instructor.objects.annotate( similarity_first=TrigramSimilarity("first_name", query), - similarity_second=TrigramSimilarity("last_name", query), + similarity_last=TrigramSimilarity("last_name", query), similarity_full=TrigramSimilarity("full_name", query), ) .annotate( max_similarity=Greatest( F("similarity_first"), - F("similarity_second"), + F("similarity_last"), F("similarity_full"), output_field=FloatField(), ) )
348-396: Consider extracting the instructor similarity logic to reduce duplication.Both
autocompleteandfetch_instructors(lines 207-238) compute instructor trigram similarities using nearly identical logic. This duplication increases maintenance burden.Consider extracting a helper function like:
def _annotate_instructor_similarity(queryset, query): """Annotate instructor queryset with trigram similarity scores.""" return queryset.annotate( similarity_first=TrigramSimilarity("first_name", query), similarity_last=TrigramSimilarity("last_name", query), similarity_full=TrigramSimilarity("full_name", query), ).annotate( max_similarity=Greatest( F("similarity_first"), F("similarity_last"), F("similarity_full"), output_field=FloatField(), ) )Then both functions can use:
instructors = _annotate_instructor_similarity(Instructor.objects, query).filter(...)tcf_website/templates/search/searchbar.html (1)
208-209: Fix inconsistent HTML attribute spacing.Lines 208-209 use
class = "..."andid = "..."with spaces around the equals sign, which is inconsistent with the rest of the file (e.g., lines 9, 10, 21 useclass="..."without spaces).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tcf_website/api/serializers.py(2 hunks)tcf_website/static/search/autocomplete.js(1 hunks)tcf_website/templates/search/searchbar.html(1 hunks)tcf_website/urls.py(2 hunks)tcf_website/views/__init__.py(1 hunks)tcf_website/views/search.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tcf_website/api/serializers.py (1)
tcf_website/models/models.py (18)
Meta(151-160)Meta(190-200)Meta(242-249)Meta(440-457)Meta(513-521)Meta(853-872)Meta(905-908)Meta(933-939)Meta(981-992)Meta(1043-1052)Meta(1093-1099)Meta(1347-1358)Meta(1387-1397)Meta(1597-1603)Meta(1625-1635)Meta(1657-1667)Course(536-872)Instructor(293-457)
tcf_website/templates/search/searchbar.html (2)
tcf_website/static/search/filters.js (2)
clearFilters(299-335)saveFiltersToLocalStorage(77-106)tcf_website/static/club/mode_toggle.js (1)
updateSearchbarState(25-47)
tcf_website/views/search.py (3)
tcf_website/static/search/autocomplete.js (2)
response(44-47)data(54-54)tcf_website/api/serializers.py (2)
CourseAutocompleteSerializer(155-164)InstructorAutocompleteSerializer(175-183)tcf_website/models/models.py (2)
Instructor(293-457)Course(536-872)
tcf_website/urls.py (1)
tcf_website/views/search.py (2)
search(76-151)autocomplete(349-396)
tcf_website/views/__init__.py (1)
tcf_website/views/browse.py (1)
club_category(548-584)
🪛 GitHub Actions: Continuous Integration
tcf_website/static/search/autocomplete.js
[error] 8-8: prettier/prettier Delete extraneous whitespace at line 8 (formatting issue detected by ESLint). Command failed: npx eslint -c .config/.eslintrc.yml tcf_website/static/
🪛 GitHub Check: eslint
tcf_website/static/search/autocomplete.js
[failure] 142-142:
Insert ,
[failure] 132-132:
Replace !suggestionsContainer.contains(event.target)·&&·event.target·!==·searchInput with ⏎······!suggestionsContainer.contains(event.target)·&&⏎······event.target·!==·searchInput⏎····
[failure] 46-46:
Insert ,
[failure] 31-31:
Insert ⏎·····
[failure] 17-17:
Delete ··
[failure] 8-8:
Delete ··
🪛 Ruff (0.14.2)
tcf_website/api/serializers.py
164-164: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
180-183: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (7)
tcf_website/views/__init__.py (1)
10-10: LGTM!The import reordering has no functional impact.
tcf_website/urls.py (1)
7-7: LGTM!The autocomplete endpoint is properly configured with a clean URL pattern and named route.
Also applies to: 131-131
tcf_website/api/serializers.py (2)
155-164: LGTM!The serializer correctly flattens the subdepartment relationship for a clean autocomplete response.
175-183: LGTM!The serializer provides the minimal fields needed for instructor autocomplete.
tcf_website/templates/search/searchbar.html (3)
10-10: LGTM!The
id="search-input"attribute correctly enables the autocomplete JavaScript to target this input element.
204-205: LGTM!The hidden inputs for
weekdaysandmin_gpaare properly configured with appropriate default values and IDs that match the JavaScript infilters.js.
217-217: LGTM!The autocomplete script is loaded after the DOM elements are in place, ensuring proper initialization.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tcf_website/views/search.py (1)
223-245: Add docstring for get_instructor_results to satisfy pylint
pylintis failing on this function (C0116: missing-function-docstring). Adding even a short descriptive docstring will clear the error and unblock CI.-@api_view(["GET"]) -def autocomplete(request): +@api_view(["GET"]) +def autocomplete(request): @@ -def get_instructor_results(query, limit: int = 10): +def get_instructor_results(query, limit: int = 10): + """Return instructors annotated with trigram similarity, limited to `limit` results."""
♻️ Duplicate comments (1)
tcf_website/static/search/autocomplete.js (1)
1-142: Blocking: fix lint/Prettier violations in autocomplete.jsCI is still red because Prettier/ESLint report whitespace and trailing-comma violations here (see failures at lines 8, 15, 43, 131, 141). Please run the project formatter (e.g.,
npx prettier --write tcf_website/static/search/autocomplete.js && npx eslint --fix tcf_website/static/search/autocomplete.js) before pushing again so the pipeline can pass.
🧹 Nitpick comments (1)
tcf_website/static/search/autocomplete.js (1)
82-118: Defensive guard before iterating suggestionsRight now we always call
.forEachondata.courses/data.instructors, which will throw if either property is ever missing or null (for example, if the backend changes shape or a proxy strips fields). Wrapping the loops in the existinghasCourses/hasInstructorschecks keeps the widget resilient.- data.courses.forEach((course) => { - const item = document.createElement("div"); - item.classList.add("autocomplete-item"); - item.style.cursor = "pointer"; - item.textContent = `${course.subdepartment} ${course.number} — ${course.title}`; - item.addEventListener("click", () => { - searchInput.value = `${course.subdepartment} ${course.number}`; - clearSuggestions(); - if (searchInput.form) { - searchInput.form.submit(); - } - }); - suggestionsContainer.appendChild(item); - }); + if (hasCourses) { + data.courses.forEach((course) => { + const item = document.createElement("div"); + item.classList.add("autocomplete-item"); + item.style.cursor = "pointer"; + item.textContent = `${course.subdepartment} ${course.number} — ${course.title}`; + item.addEventListener("click", () => { + searchInput.value = `${course.subdepartment} ${course.number}`; + clearSuggestions(); + if (searchInput.form) { + searchInput.form.submit(); + } + }); + suggestionsContainer.appendChild(item); + }); + } … - data.instructors.forEach((instructor) => { - const item = document.createElement("div"); - item.classList.add("autocomplete-item"); - item.style.cursor = "pointer"; - item.textContent = instructor.full_name; - item.addEventListener("click", () => { - searchInput.value = instructor.full_name; - clearSuggestions(); - if (searchInput.form) { - searchInput.form.submit(); - } - }); - suggestionsContainer.appendChild(item); - }); + if (hasInstructors) { + data.instructors.forEach((instructor) => { + const item = document.createElement("div"); + item.classList.add("autocomplete-item"); + item.style.cursor = "pointer"; + item.textContent = instructor.full_name; + item.addEventListener("click", () => { + searchInput.value = instructor.full_name; + clearSuggestions(); + if (searchInput.form) { + searchInput.form.submit(); + } + }); + suggestionsContainer.appendChild(item); + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tcf_website/static/search/autocomplete.js(1 hunks)tcf_website/views/search.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tcf_website/views/search.py (2)
tcf_website/static/search/autocomplete.js (2)
response(41-44)data(51-51)tcf_website/api/serializers.py (2)
CourseAutocompleteSerializer(155-164)InstructorAutocompleteSerializer(175-183)
🪛 GitHub Actions: Continuous Integration
tcf_website/views/search.py
[error] 223-223: pylint: C0116: Missing function or method docstring (missing-function-docstring)
[error] 223-223: pylint reported a failure for the file due to missing docstring at the specified line.
tcf_website/static/search/autocomplete.js
[error] 8-8: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
🪛 GitHub Check: eslint
tcf_website/static/search/autocomplete.js
[failure] 141-141:
Insert ,
[failure] 131-131:
Replace !suggestionsContainer.contains(event.target)·&&·event.target·!==·searchInput with ⏎······!suggestionsContainer.contains(event.target)·&&⏎······event.target·!==·searchInput⏎····
[failure] 43-43:
Insert ,
[failure] 15-15:
Delete ··
[failure] 8-8:
Delete ··
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tcf_website/static/search/autocomplete.js (1)
15-145: Fix remaining ESLint/Prettier formatting violations so CI passesESLint/Prettier are still complaining (per hints) around:
- Line 15: stray spaces on a blank line.
- Line 43: missing trailing comma in the multi-line
fetchcall’s arguments.- Line 135: long conditional that Prettier wants split across lines.
- Line 145: missing trailing comma in the multi-line
addEventListenercall.The simplest fix is to run the project formatter on this file:
npx prettier --write tcf_website/static/search/autocomplete.js # and/or, if configured: npx eslint --fix tcf_website/static/search/autocomplete.jsThen commit the updated file so the CI lint step goes green.
🧹 Nitpick comments (1)
tcf_website/static/search/autocomplete.js (1)
9-14: Avoid silently disabling autocomplete when.searchbar-wrapperis missingRight now, if
searchInput.closest(".searchbar-wrapper")returnsnull, the scriptreturns and autocomplete is completely disabled without any signal. That’s brittle if the HTML changes or if there are multiple search bars.Consider either:
- Falling back to
searchInput.parentElement(or another known container), or- Logging a warning so it’s obvious in dev tools when the wrapper is missing.
That will make the feature more resilient to template changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tcf_website/static/search/autocomplete.js(1 hunks)tcf_website/static/search/searchbar.css(1 hunks)tcf_website/templates/search/searchbar.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tcf_website/templates/search/searchbar.html
🧰 Additional context used
🪛 GitHub Actions: Continuous Integration
tcf_website/static/search/autocomplete.js
[error] 15-15: Prettier formatting check failed in tcf_website/static/search/autocomplete.js. Run 'prettier --write' to fix code style issues.
🪛 GitHub Check: eslint
tcf_website/static/search/autocomplete.js
[failure] 145-145:
Insert ,
[failure] 135-135:
Replace !suggestionsContainer.contains(event.target)·&&·event.target·!==·searchInput with ⏎······!suggestionsContainer.contains(event.target)·&&⏎······event.target·!==·searchInput⏎····
[failure] 43-43:
Insert ,
[failure] 15-15:
Delete ··
🔇 Additional comments (1)
tcf_website/static/search/autocomplete.js (1)
1-147: Autocomplete logic and error handling look solidNice job wiring this up: debounced input, AbortController-based cancellation, safe
textContentusage, grouped headers, and outside-click handling all look correct and cohesive with the/api/autocomplete/endpoint. No functional issues stand out in the core flow.
|
|
||
| /* Autocomplete suggestions positioning */ | ||
| .searchbar-wrapper { | ||
| position: relative; | ||
| } | ||
|
|
||
| /* Remove focus outline from search input */ | ||
| #search-input:focus { | ||
| outline: none !important; | ||
| box-shadow: none !important; | ||
| border-color: inherit; | ||
| } | ||
|
|
||
| .autocomplete-suggestions { | ||
| position: absolute; | ||
| top: 100%; | ||
| left: 0; | ||
| right: 0; | ||
| z-index: 1000; | ||
| background: white; | ||
| border: 1px solid #e2e8f0; | ||
| border-radius: 6px; | ||
| box-shadow: 0 2px 8px rgba(0, 0, 0, 0.1); | ||
| margin-top: 0; | ||
| max-height: 400px; | ||
| overflow: hidden; | ||
| display: none; | ||
| padding: 0.5rem 0; | ||
| } | ||
|
|
||
| .autocomplete-header { | ||
| padding: 0.75rem 1rem; | ||
| font-weight: 600; | ||
| font-size: 0.875rem; | ||
| color: #4a5568; | ||
| background-color: #f7fafc; | ||
| border-bottom: 1px solid #e2e8f0; | ||
| margin: 0; | ||
| } | ||
|
|
||
| .autocomplete-item { | ||
| padding: 0.75rem 1rem; | ||
| cursor: pointer; | ||
| border-bottom: 1px solid #f7fafc; | ||
| margin: 0; | ||
| transition: background-color 0.15s ease; | ||
| } | ||
|
|
||
| .autocomplete-item:hover { | ||
| background-color: #f7fafc; | ||
| } | ||
|
|
||
| .autocomplete-item:last-child { | ||
| border-bottom: none; | ||
| } |
There was a problem hiding this comment.
Restore visible focus indication and allow dropdown scrolling
#search-input:focus removes both outline and box-shadow with !important, which effectively hides focus state for keyboard users and is an accessibility regression. Also, .autocomplete-suggestions uses overflow: hidden, so taller result sets will be clipped instead of scrollable.
Consider:
-/* Remove focus outline from search input */
-#search-input:focus {
- outline: none !important;
- box-shadow: none !important;
- border-color: inherit;
-}
+/* Provide a visible, accessible focus style for the search input */
+#search-input:focus {
+ outline: 2px solid #d75626;
+ outline-offset: 2px;
+}and:
- max-height: 400px;
- overflow: hidden;
+ max-height: 400px;
+ overflow-y: auto;This keeps the custom look while preserving keyboard focus visibility and avoiding clipped suggestions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tcf_website/views/search.py (3)
207-221: Instructor search refactor looks good; consider relocating the threshold commentCentralizing the instructor similarity query in
get_instructor_resultsand lettingfetch_instructorsjust map to simple dicts is clean and keeps the logic reusable for both the HTML search view and the API.Minor nit: the
# arbitrarily chosen thresholdcomment now lives infetch_instructors, but the actualsimilarity_threshold = 0.5is defined inget_instructor_results. Moving that comment down next tosimilarity_threshold(or removing it) would avoid confusion.Also applies to: 223-246
248-260: Tightenlimithandling and double‑check type‑hint compatibilityThe
normalize_search_queryhelper is a nice way to makeCS3100match the indexedCS 3100pattern.Two small suggestions around
limit:
if limit:will silently ignorelimit=0(treating it as “no limit”). Using an explicitis not Nonecheck is clearer and avoids that edge case:-def fetch_courses(query, filters, limit: int | None = None): +def fetch_courses(query, filters, limit: int | None = None): @@ - if limit: + if limit is not None: results = results[:limit]
- The
int | Noneannotation requires Python 3.10+ unless you’re usingfrom __future__ import annotations. If the project targets earlier versions, consider switching toOptional[int]instead (and importing it), or ensuring the runtime is 3.10+.For example, if you want broader compatibility:
-from typing import Iterable +from typing import Iterable, Optional @@ -def fetch_courses(query, filters, limit: int | None = None): +def fetch_courses(query, filters, limit: Optional[int] = None): @@ - if limit: + if limit is not None: results = results[:limit]Also applies to: 295-297
352-370: Autocomplete endpoint wiring is correct; consider a minimum query length for load controlThe DRF
autocompleteview correctly:
- Reads
qfrom the query string.- Short‑circuits to empty results when there’s no input.
- Uses the shared instructor/course query helpers.
- Serializes with the new autocomplete serializers.
Given this will likely be called on each keypress, you may want to enforce a minimal query length (e.g., 2–3 characters) before hitting the database, to protect against unnecessary load from very short queries:
- if not query: - return Response({"courses": [], "instructors": []}) # empty list if no input + # Avoid DB hits for empty or very short queries; adjust length threshold as needed. + if len(query) < 2: + return Response({"courses": [], "instructors": []})This is optional, but can help in production traffic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tcf_website/static/search/autocomplete.js(1 hunks)tcf_website/urls.py(2 hunks)tcf_website/views/__init__.py(1 hunks)tcf_website/views/search.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tcf_website/urls.py
- tcf_website/static/search/autocomplete.js
🧰 Additional context used
🧬 Code graph analysis (2)
tcf_website/views/__init__.py (1)
tcf_website/views/browse.py (1)
club_category(498-534)
tcf_website/views/search.py (1)
tcf_website/api/serializers.py (2)
CourseAutocompleteSerializer(155-164)InstructorAutocompleteSerializer(175-183)
🔇 Additional comments (2)
tcf_website/views/__init__.py (1)
7-15: Exposingclub_categoryvia the views package is consistent and safeAdding
club_categoryto the browse import block keeps the publictcf_website.viewssurface consistent with other views and removes duplication without affecting behavior. Change looks good.tcf_website/views/search.py (1)
8-18: New paginator/DRF/serializer imports align with usageThe added imports (paginator classes,
api_view,Response, and the autocomplete serializers) are all used below and match the patterns in the rest of the module. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tcf_website/static/search/autocomplete.js (1)
49-52: Use ES6 property shorthand for cleaner code.The
signalproperty can use ES6 shorthand syntax since the property name and variable name are identical.🔎 Proposed fix
const response = await fetch( `/api/autocomplete/?q=${encodeURIComponent(query)}&mode=${mode}`, - { signal: signal }, + { signal }, );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tcf_website/api/serializers.pytcf_website/static/search/autocomplete.jstcf_website/templates/club/mode_toggle.htmltcf_website/templates/search/searchbar.htmltcf_website/urls.pytcf_website/views/__init__.pytcf_website/views/search.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tcf_website/urls.py
- tcf_website/views/init.py
🧰 Additional context used
🧬 Code graph analysis (3)
tcf_website/templates/club/mode_toggle.html (1)
tcf_website/static/club/mode_toggle.js (1)
updateSearchbarState(25-47)
tcf_website/api/serializers.py (1)
tcf_website/models/models.py (18)
Meta(151-160)Meta(190-200)Meta(242-249)Meta(507-524)Meta(580-588)Meta(921-940)Meta(963-966)Meta(991-997)Meta(1049-1060)Meta(1111-1120)Meta(1368-1379)Meta(1408-1418)Meta(1618-1624)Meta(1646-1656)Meta(1678-1688)Course(603-940)Instructor(293-524)Club(219-249)
tcf_website/views/search.py (5)
tcf_website/static/search/autocomplete.js (6)
response(49-52)instructors(84-84)mode(46-46)clubs(85-85)data(59-59)courses(83-83)tcf_website/api/serializers.py (3)
CourseAutocompleteSerializer(155-164)InstructorAutocompleteSerializer(175-183)ClubAutocompleteSerializer(204-215)tcf_website/static/search/filters.js (2)
filters(94-102)filters(114-114)tcf_website/views/browse.py (1)
parse_mode(150-153)tcf_website/views/review.py (1)
parse_mode(23-26)
🪛 GitHub Actions: Continuous Integration
tcf_website/views/search.py
[error] 361-361: pylint: W0612 Unused variable 'mode' (unused-variable).
🪛 GitHub Check: eslint
tcf_website/static/search/autocomplete.js
[warning] 51-51:
Expected property shorthand
🪛 Ruff (0.14.10)
tcf_website/api/serializers.py
164-164: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
180-183: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tcf_website/views/search.py
361-361: Unpacked variable mode is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (6)
tcf_website/templates/search/searchbar.html (1)
5-212: LGTM! Searchbar restructuring properly supports autocomplete.The searchbar has been successfully restructured to accommodate the autocomplete feature:
- Added
searchbar-wrappercontainer for proper positioning of the dropdown- Added
id="search-input"for the autocomplete script hook- Integrated mode toggle and search button inline
- Included
autocomplete.jsscript referenceThe layout changes align well with the autocomplete requirements and maintain proper form structure.
tcf_website/api/serializers.py (1)
155-215: LGTM! Autocomplete serializers are well-designed.The three new autocomplete serializers are properly implemented:
- CourseAutocompleteSerializer: Exposes minimal fields (id, title, number, subdepartment mnemonic) needed for autocomplete suggestions
- InstructorAutocompleteSerializer: Returns id and full_name for instructor suggestions
- ClubAutocompleteSerializer: Provides id, name, and category_name for club suggestions
The use of
sourceparameter to access related model fields (subdepartment.mnemonic,category.name) is correct and efficient.tcf_website/views/search.py (4)
155-196: LGTM! fetch_clubs properly implements limit parameter.The addition of the
limitparameter and its application after building the queryset is correct. The threshold-based filtering is preserved, and the limit is applied at the end as expected for autocomplete results.
212-250: LGTM! Instructor fetching refactored with shared helper.The refactoring to extract
get_instructor_resultsis a good improvement:
- Reduces code duplication between
fetch_instructorsand the autocomplete endpoint- Maintains the same trigram similarity logic
- Properly applies the limit parameter
253-303: LGTM! fetch_courses properly implements limit parameter.The addition of the
limitparameter is correctly applied after filtering and ordering, which ensures:
- Search normalization is still performed
- Filters are applied before limiting
- Similarity threshold is respected
- Results are ordered by relevance before truncation
357-382: Verify autocomplete endpoint behavior and consider empty clubs response.The autocomplete endpoint implementation looks solid overall. However, there's a minor inconsistency: when in club mode (line 368-370), the response only includes
clubs, but the frontend autocomplete.js expects all three keys (courses,instructors,clubs) based on lines 73-75 and 82-84.While the frontend uses optional chaining and defaults to empty arrays (
data?.courses?.length > 0,data.courses || []), it would be more consistent to always return all three keys for predictable API behavior.Consider updating the club mode response for consistency:
if is_club: clubs = fetch_clubs(query, limit=5) - return Response({"clubs": ClubAutocompleteSerializer(clubs, many=True).data}) + return Response({ + "courses": [], + "instructors": [], + "clubs": ClubAutocompleteSerializer(clubs, many=True).data + })Alternatively, verify that the frontend properly handles partial responses in all cases.
|
|
||
|
|
||
| class ClubAutocompleteSerializer(serializers.ModelSerializer): | ||
| """DEF Serializer for Club autocomplete""" |
There was a problem hiding this comment.
Fix typo in docstring.
The docstring has "DEF Serializer" but should be "DRF Serializer" (Django REST Framework).
🔎 Proposed fix
- """DEF Serializer for Club autocomplete"""
+ """DRF Serializer for Club autocomplete"""📝 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.
| """DEF Serializer for Club autocomplete""" | |
| """DRF Serializer for Club autocomplete""" |
🤖 Prompt for AI Agents
In tcf_website/api/serializers.py around line 205, the docstring contains a typo
"DEF Serializer for Club autocomplete"; update the docstring to read "DRF
Serializer for Club autocomplete" to correctly reference Django REST Framework
and save the file.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tcf_website/views/search.py (2)
155-197: Apply limit to queryset before materialization for better performance.The limit is currently applied at line 195 after the queryset has been fully evaluated and converted to a list of dictionaries (lines 182-192). For autocomplete with
limit=5, this means fetching and processing all matching clubs from the database, then discarding most of them.🔎 Proposed refactor to apply limit earlier
def fetch_clubs(query, limit: int | None = None): """Get club data using Django Trigram similarity.""" threshold = 0.15 if not query: - return list( - Club.objects.annotate( + qs = Club.objects.annotate( max_similarity=Value(1.0, output_field=FloatField()), category_slug=F("category__slug"), category_name=F("category__name"), - ).values( + ) + if limit: + qs = qs[:limit] + return list( + qs.values( "id", "name", "description", "max_similarity", "category_slug", "category_name", ) ) qs = ( Club.objects.annotate(sim=TrigramSimilarity("combined_name", query)) .annotate(max_similarity=F("sim")) .filter(max_similarity__gte=threshold) .annotate(category_slug=F("category__slug"), category_name=F("category__name")) .order_by("-max_similarity") ) + + if limit: + qs = qs[:limit] qs = [ { "id": c.id, "name": c.name, "description": c.description, "max_similarity": c.max_similarity, "category_slug": c.category_slug, "category_name": c.category_name, } for c in qs ] - if limit: - qs = qs[:limit] return qs
357-382: Consider adding error handling for production robustness.The autocomplete endpoint currently has no error handling for database exceptions, serialization errors, or other potential failures. While DRF provides some default error handling, explicitly catching and logging exceptions would improve debuggability and allow for more graceful degradation.
💡 Example error handling pattern
@api_view(["GET"]) def autocomplete(request): """implement autocomplete for search bar""" try: query = request.GET.get("q", "").strip() _, is_club = parse_mode(request) if not query: return Response( {"courses": [], "instructors": [], "clubs": []} ) if is_club: clubs = fetch_clubs(query, limit=5) return Response({"clubs": ClubAutocompleteSerializer(clubs, many=True).data}) instructors = get_instructor_results(query, limit=5) courses = fetch_courses(query, filters={}, limit=5) return Response( { "courses": CourseAutocompleteSerializer(courses, many=True).data, "instructors": InstructorAutocompleteSerializer( instructors, many=True ).data, } ) except Exception as e: # Log the error for debugging logger.exception("Autocomplete error for query: %s", query) # Return empty results rather than 500 error return Response( {"courses": [], "instructors": [], "clubs": []}, status=200 )Note: You'll need to add
import loggingandlogger = logging.getLogger(__name__)at the top of the file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tcf_website/views/search.py
🔇 Additional comments (4)
tcf_website/views/search.py (4)
12-19: LGTM! DRF integration looks correct.The imports for Django REST Framework decorators, Response, and the new autocomplete serializers are properly structured for the new API endpoint.
212-226: LGTM! Clean refactoring.The extraction of query logic into
get_instructor_resultsand the use of a dict comprehension withgetattrmakes the code more maintainable and testable.
228-250: LGTM! Efficient implementation.The use of
.only()to fetch minimal fields and applying the limit via queryset slicing ensures the database query includes a SQL LIMIT clause, making this efficient for autocomplete.
253-303: LGTM! Limit applied correctly.The limit is applied to the queryset after filtering and ordering (lines 300-301), which ensures Django translates it to a SQL LIMIT clause for efficient autocomplete queries.
| @api_view(["GET"]) | ||
| def autocomplete(request): | ||
| """implement autocomplete for search bar""" | ||
| query = request.GET.get("q", "").strip() | ||
| _, is_club = parse_mode(request) | ||
|
|
||
| if not query: | ||
| return Response( | ||
| {"courses": [], "instructors": [], "clubs": []} | ||
| ) # empty list if no input | ||
|
|
||
| if is_club: | ||
| clubs = fetch_clubs(query, limit=5) | ||
| return Response({"clubs": ClubAutocompleteSerializer(clubs, many=True).data}) | ||
|
|
||
| instructors = get_instructor_results(query, limit=5) | ||
| courses = fetch_courses(query, filters={}, limit=5) | ||
|
|
||
| return Response( | ||
| { | ||
| "courses": CourseAutocompleteSerializer(courses, many=True).data, | ||
| "instructors": InstructorAutocompleteSerializer( | ||
| instructors, many=True | ||
| ).data, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Fix inconsistent response structure across modes.
The autocomplete endpoint returns different response structures depending on mode:
- Empty query (lines 364-366):
{"courses": [], "instructors": [], "clubs": []} - Club mode (line 370):
{"clubs": [...]} - Course mode (lines 375-381):
{"courses": [...], "instructors": [...]}
This inconsistency forces clients to handle different response shapes, complicating frontend logic and increasing the likelihood of bugs. For example, accessing response.courses in club mode would fail.
🔎 Proposed fix for consistent response structure
if is_club:
clubs = fetch_clubs(query, limit=5)
- return Response({"clubs": ClubAutocompleteSerializer(clubs, many=True).data})
+ return Response({
+ "courses": [],
+ "instructors": [],
+ "clubs": ClubAutocompleteSerializer(clubs, many=True).data
+ })
instructors = get_instructor_results(query, limit=5)
courses = fetch_courses(query, filters={}, limit=5)
return Response(
{
"courses": CourseAutocompleteSerializer(courses, many=True).data,
"instructors": InstructorAutocompleteSerializer(
instructors, many=True
).data,
+ "clubs": [],
}
)🤖 Prompt for AI Agents
In tcf_website/views/search.py around lines 357 to 382, the autocomplete
endpoint returns different response shapes per mode; change it to always return
the same keys ("courses", "instructors", "clubs") so clients can rely on a
consistent structure. Specifically, when query is empty return {"courses": [],
"instructors": [], "clubs": []}; when is_club is True return {"clubs":
ClubAutocompleteSerializer(clubs, many=True).data, "courses": [], "instructors":
[]}; and when course/instructor mode return the current {"courses":
CourseAutocompleteSerializer(...).data, "instructors":
InstructorAutocompleteSerializer(...).data, "clubs": []}. Ensure serializers are
used for present lists and empty lists are plain [].
artiehumphreys
force-pushed
the
autocomplete
branch
from
53cd8c2 to
e65d41f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tcf_website/api/serializers.py (1)
205-205: Fix typo in docstring.The docstring contains "DEF Serializer" but should be "DRF Serializer" (Django REST Framework).
🔎 Proposed fix
- """DEF Serializer for Club autocomplete""" + """DRF Serializer for Club autocomplete"""tcf_website/views/search.py (1)
357-382: Fix inconsistent response structure across modes.The autocomplete endpoint returns different response structures depending on mode:
- Empty query:
{"courses": [], "instructors": [], "clubs": []}- Club mode:
{"clubs": [...]}- Course mode:
{"courses": [...], "instructors": [...]}This inconsistency forces clients to handle different response shapes, complicating frontend logic and increasing the likelihood of bugs (e.g., accessing
response.coursesin club mode would fail).🔎 Proposed fix for consistent response structure
if is_club: clubs = fetch_clubs(query, limit=5) - return Response({"clubs": ClubAutocompleteSerializer(clubs, many=True).data}) + return Response({ + "courses": [], + "instructors": [], + "clubs": ClubAutocompleteSerializer(clubs, many=True).data + }) instructors = get_instructor_results(query, limit=5) courses = fetch_courses(query, filters={}, limit=5) return Response( { "courses": CourseAutocompleteSerializer(courses, many=True).data, "instructors": InstructorAutocompleteSerializer( instructors, many=True ).data, + "clubs": [], } )
🧹 Nitpick comments (1)
tcf_website/static/search/autocomplete.js (1)
49-52: Use ES6 property shorthand.Line 51 can use ES6 property shorthand syntax for cleaner code.
🔎 Proposed refactor
const response = await fetch( `/api/autocomplete/?q=${encodeURIComponent(query)}&mode=${mode}`, - { signal: signal }, + { signal }, );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tcf_website/api/serializers.pytcf_website/static/search/autocomplete.jstcf_website/static/search/searchbar.csstcf_website/templates/club/mode_toggle.htmltcf_website/templates/search/searchbar.htmltcf_website/urls.pytcf_website/views/__init__.pytcf_website/views/search.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tcf_website/static/search/searchbar.css
- tcf_website/urls.py
🧰 Additional context used
🧬 Code graph analysis (3)
tcf_website/views/__init__.py (2)
tcf_website/views/browse.py (1)
club_category(498-534)tcf_website/views/search.py (2)
search(77-152)autocomplete(358-382)
tcf_website/views/search.py (3)
tcf_website/api/serializers.py (3)
CourseAutocompleteSerializer(155-164)InstructorAutocompleteSerializer(175-183)ClubAutocompleteSerializer(204-215)tcf_website/views/browse.py (1)
parse_mode(150-153)tcf_website/views/review.py (1)
parse_mode(23-26)
tcf_website/api/serializers.py (1)
tcf_website/models/models.py (18)
Meta(151-160)Meta(190-200)Meta(242-249)Meta(507-524)Meta(580-588)Meta(921-940)Meta(963-966)Meta(991-997)Meta(1049-1060)Meta(1111-1120)Meta(1368-1379)Meta(1408-1418)Meta(1618-1624)Meta(1646-1656)Meta(1678-1688)Course(603-940)Instructor(293-524)Club(219-249)
🪛 GitHub Check: eslint
tcf_website/static/search/autocomplete.js
[warning] 51-51:
Expected property shorthand
🪛 Ruff (0.14.10)
tcf_website/api/serializers.py
164-164: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
180-183: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (10)
tcf_website/templates/club/mode_toggle.html (1)
16-20: LGTM! Previous duplicate parameter issue resolved.The hidden input now correctly uses only the
idattribute without anameattribute, avoiding the duplicate form parameter issue while still providing JavaScript access to the current mode value.tcf_website/views/__init__.py (1)
9-9: LGTM! Clean API surface expansion.The new exports (
club_categoryandautocomplete) correctly expose the newly added view functions for public use.Also applies to: 42-42
tcf_website/templates/search/searchbar.html (1)
5-212: LGTM! Well-structured autocomplete integration.The template changes properly support the new autocomplete feature by adding the necessary wrapper, input ID, hidden fields, and script inclusion while maintaining the existing filter functionality.
tcf_website/api/serializers.py (2)
155-165: LGTM! Correct serializer implementation.The
CourseAutocompleteSerializerappropriately exposes the minimal fields needed for autocomplete and correctly usessource="subdepartment.mnemonic"to access the related subdepartment's mnemonic.
175-184: LGTM! Simple and correct serializer.The
InstructorAutocompleteSerializercorrectly exposes only the essential fields (idandfull_name) needed for instructor autocomplete.tcf_website/views/search.py (5)
12-19: LGTM! Clean import additions.The new imports for DRF decorators, Response, and autocomplete serializers are properly structured to support the new API endpoint.
155-196: LGTM! Clean limit support added.The
fetch_clubsfunction correctly adds optionallimitparameter while maintaining backward compatibility for existing callers.
212-226: LGTM! Good refactoring for code reuse.The
fetch_instructorsrefactor appropriately delegates to the newget_instructor_resultshelper, enabling code reuse for the autocomplete endpoint while maintaining the existing return structure.
228-251: LGTM! Well-implemented search helper.The
get_instructor_resultshelper correctly computes trigram similarity across multiple name fields, applies the limit efficiently in the queryset, and returns results suitable for both autocomplete and search use cases.
253-303: LGTM! Backward-compatible limit support.The
fetch_coursesfunction properly adds optionallimitparameter while preserving existing behavior for callers that don't specify a limit.
3 participants
GitHub Issues addressed
What I did
Screenshots
Testing
Questions/Discussions/Notes
Summary by CodeRabbit
New Features
UI Updates
✏️ Tip: You can customize this high-level summary in your review settings.