Teditor#18
Conversation
…ian) to connect a book search to closest fuzzy match
…nd pdf parser now both work
📝 WalkthroughWalkthroughThis PR introduces a new Librarian agent for searching books via Open Library, adds a BookSearch React component for book discovery, refactors the frontend App and MapComponent with updated styling and props, removes mood metadata from literary data, and simplifies the conductor orchestration by eliminating chain-of-thought instrumentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Frontend as BookSearch Component
participant Backend as Librarian API
participant OpenLib as Open Library API
User->>Frontend: Enter search query
Frontend->>Frontend: Debounce (400ms)
Frontend->>Backend: POST /tools/librarian/search<br/>(query, limit)
Backend->>OpenLib: GET search.json?title=query
OpenLib-->>Backend: Book results with metadata
Backend->>Backend: Extract & normalize fields<br/>(title, authors, cover_url, etc.)
Backend-->>Frontend: JSON response with book list
Frontend->>Frontend: Display results<br/>(images, titles, authors, years)
User->>Frontend: Select a book
Frontend->>Frontend: Highlight selection
Frontend->>User: Trigger onBookSelect callback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new LibrarianAgent (Open Library search tool) and integrates it into the existing MCP orchestration + frontend UI, alongside a fairly large refactor of the map UI and Conductor responses.
Changes:
- Added a Django
librarianapp exposing/tools/librarian/search(and wired it intocoresettings/urls and the index agent list). - Updated the Conductor to support a
"search"action shortcut and simplified orchestration responses (removed chain-of-thought). - Frontend: added a new BookSearch widget + significant MapComponent refactor (routes/labels/filtering), and adjusted Mapbox CSS loading/proxy config.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| mcp-servers/librarian/views.py | Implements Open Library search tool + HTTP endpoint. |
| mcp-servers/librarian/urls.py | Wires search endpoint for the new agent. |
| mcp-servers/librarian/apps.py | Registers the Django app config. |
| mcp-servers/librarian/init.py | Initializes the librarian module/package. |
| mcp-servers/core/views.py | Adds LibrarianAgent metadata to the index response. |
| mcp-servers/core/urls.py | Includes librarian.urls under /tools/librarian/. |
| mcp-servers/core/settings.py | Adds librarian to INSTALLED_APPS. |
| mcp-servers/core/conductor.py | Adds "action":"search" path + removes chain-of-thought + tweaks timeline handling. |
| mcp-servers/archivist/knowledge_base.py | Removes mood fields from knowledge base entries. |
| frontend/vite.config.js | Proxies /orchestrate to the Django backend in dev. |
| frontend/src/main.jsx | Imports Mapbox GL CSS via bundler. |
| frontend/src/data/literaryPoints.js | Removes mood fields from map data points. |
| frontend/src/components/MapComponent.jsx | Major map refactor (route lines, labels, filters, stylist overrides, upload layer changes). |
| frontend/src/components/BookSearch.jsx | New UI component to query LibrarianAgent + display selectable results. |
| frontend/src/api/archivistClient.js | Adds fetchLibrarianSearch and refactors upload + removes VibeSearch helper. |
| frontend/src/App.jsx | Integrates BookSearch + updated upload wiring + removes vibe search/detective UI, passes new map props. |
| frontend/index.html | Updates Mapbox CSS link version and page sizing styles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,6 @@ | |||
| import React from "react"; | |||
| import ReactDOM from "react-dom/client"; | |||
| import "mapbox-gl/dist/mapbox-gl.css"; | |||
There was a problem hiding this comment.
Mapbox CSS is now imported here, but frontend/index.html also includes Mapbox CSS from a CDN. This can lead to duplicated CSS and version mismatches. Prefer a single approach (bundled import or CDN), and keep CSS/JS versions aligned.
| import "mapbox-gl/dist/mapbox-gl.css"; |
| 📄 Upload Book PDF — AI Location Extraction | ||
| </p> | ||
| <BookUpload | ||
| accentColor={eraColor} |
There was a problem hiding this comment.
BookUpload doesn’t accept or use an accentColor prop, but it’s passed here. Either remove this prop, or update BookUpload to support theme coloring to avoid dead/incorrect props.
| accentColor={eraColor} |
| /* ── Book selection from LibrarianAgent search ──────────────── */ | ||
| const handleBookSelect = useCallback((book) => { | ||
| setSelectedBook(book); | ||
| console.log("Selected book for processing:", book); |
There was a problem hiding this comment.
Leftover debug logging (console.log) will spam the console in normal usage. Consider removing it or gating it behind a dev flag.
| console.log("Selected book for processing:", book); | |
| if (process.env.NODE_ENV !== "production") { | |
| console.log("Selected book for processing:", book); | |
| } |
| useEffect(() => { | ||
| if (mapRef.current) return; | ||
|
|
||
| const map = new mapboxgl.Map({ | ||
| container: mapContainer.current, | ||
| style: MAP_STYLES.Satellite, | ||
| center: [-95.7, 37.0], | ||
| zoom: 3.5, | ||
| pitch: 15, | ||
| }); |
There was a problem hiding this comment.
Map initialization runs even when MAPBOX_TOKEN is empty, but in that case the component returns a fallback UI that never renders the map container. This means mapContainer.current will be null and new mapboxgl.Map({ container: null }) can throw at runtime. Guard the init effect (and/or the component) so the map is only created when a token exists and the container ref is available.
| import json | ||
| import time | ||
| from concurrent.futures import ThreadPoolExecutor | ||
| from concurrent.futures import ThreadPoolExecutor, as_completed |
There was a problem hiding this comment.
as_completed is imported but not used, which will trigger lint/static-analysis failures. Either remove the import or iterate futures via as_completed (which also avoids waiting in submission order).
| from concurrent.futures import ThreadPoolExecutor, as_completed | |
| from concurrent.futures import ThreadPoolExecutor |
| if action == "search": | ||
| query = body.get("query") | ||
| if not query: | ||
| return JsonResponse({"error": "query is required for search action"}, status=400) | ||
| limit = body.get("limit", 10) | ||
| t0 = time.perf_counter() | ||
| try: | ||
| result = _librarian_search(query, limit=limit) | ||
| elapsed = round((time.perf_counter() - t0) * 1000) | ||
| total = round((time.perf_counter() - t_start) * 1000) | ||
| return JsonResponse({ | ||
| "librarian": result, | ||
| "timeline": [{ | ||
| "agent": "LibrarianAgent", | ||
| "tool": "search_books", | ||
| "status": "success", | ||
| "elapsed_ms": elapsed, | ||
| }], | ||
| "total_ms": total, | ||
| }) | ||
| except Exception as exc: | ||
| elapsed = round((time.perf_counter() - t0) * 1000) | ||
| total = round((time.perf_counter() - t_start) * 1000) | ||
| return JsonResponse({ | ||
| "error": str(exc), | ||
| "timeline": [{ | ||
| "agent": "LibrarianAgent", | ||
| "tool": "search_books", | ||
| "status": "error", | ||
| "elapsed_ms": elapsed, | ||
| "error": str(exc), | ||
| }], | ||
| "total_ms": total, | ||
| }, status=502) |
There was a problem hiding this comment.
For the action == "search" path, all exceptions are turned into a 502. This misclassifies client errors (e.g. whitespace-only query causes _librarian_search to raise ValueError) and also exposes raw exception strings. Mirror librarian.views.search handling here: return 400 for ValueError, 502 for httpx request/status errors, and use a generic message for unexpected exceptions.
| m.on("click", "literary-markers", (e) => { | ||
| const feature = e.features[0]; | ||
| const coords = feature.geometry.coordinates.slice(); | ||
| m.flyTo({ center: coords, zoom: 14, speed: 1.2, pitch: 45 }); | ||
| if (onMarkerClickRef.current) onMarkerClickRef.current(feature); | ||
| }); | ||
| m.on("mouseenter", "literary-markers", () => { | ||
| m.getCanvas().style.cursor = "pointer"; | ||
| }); | ||
| m.on("mouseleave", "literary-markers", () => { | ||
| m.getCanvas().style.cursor = ""; | ||
| }); |
There was a problem hiding this comment.
addAllLayers registers Mapbox event handlers (map.on(...)) each time it runs. Because it is invoked on every style.load, switching styles can accumulate duplicate click/hover handlers on the same layer ids. Consider registering handlers once (outside addAllLayers) or calling map.off before re-registering.
| // Click handlers | ||
| map.on("click", "uploaded-markers", (e) => { | ||
| const feature = e.features[0]; | ||
| const coords = feature.geometry.coordinates.slice(); | ||
| map.flyTo({ center: coords, zoom: 14, speed: 1.2, pitch: 45 }); | ||
| if (onMarkerClickRef.current) onMarkerClickRef.current(feature); | ||
| }); | ||
|
|
||
| map.on("mouseenter", "uploaded-markers", () => { | ||
| map.getCanvas().style.cursor = "pointer"; | ||
| }); | ||
| map.on("mouseleave", "uploaded-markers", () => { | ||
| map.getCanvas().style.cursor = ""; | ||
| }); |
There was a problem hiding this comment.
This effect adds new click/mouseenter/mouseleave handlers for uploaded-markers each time uploadedBookLocations changes, but it never removes prior handlers. If the user uploads multiple times, clicks can fire multiple times. Use map.off with stable handler functions (or register once and update via refs) when re-adding the layer.
| @@ -137,48 +141,24 @@ def cot(agent, step, detail=""): | |||
|
|
|||
| for key, future in futures.items(): | |||
There was a problem hiding this comment.
The current loop waits on future.result() in dict insertion order, which can block on a slow agent even if others have already completed. Since as_completed is already imported, consider iterating with it and mapping futures back to keys so timeline entries are recorded as tasks finish (and you don’t unnecessarily delay processing).
| for key, future in futures.items(): | |
| # Process futures as they complete to avoid blocking on slower agents | |
| future_to_key = {future: key for key, future in futures.items()} | |
| for future in as_completed(futures.values()): | |
| key = future_to_key[future] |
| <link | ||
| href="https://api.mapbox.com/mapbox-gl-js/v3.3.0/mapbox-gl.css" | ||
| href="https://api.mapbox.com/mapbox-gl-js/v3.18.1/mapbox-gl.css" | ||
| rel="stylesheet" | ||
| /> |
There was a problem hiding this comment.
The app now imports Mapbox CSS via mapbox-gl/dist/mapbox-gl.css (bundled), but this file still links to a CDN Mapbox CSS version (and the version differs from mapbox-gl in package.json). To avoid duplicated/mismatched styles, prefer one source: either remove this <link> and rely on the bundled CSS, or align/pin versions so CSS and JS match.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp-servers/core/views.py (1)
8-22:⚠️ Potential issue | 🟡 MinorUpdate discovery metadata to match the new agent count.
The payload still claims “3 Specialist MCP Agents,” but LibrarianAgent was added. This can confuse clients that surface the discovery metadata.🛠️ Suggested fix
- "architecture": "Conductor → 3 Specialist MCP Agents", + "architecture": "Conductor → 4 Specialist MCP Agents (incl. Librarian)",
🤖 Fix all issues with AI agents
In `@frontend/index.html`:
- Around line 12-14: The CSS sets `#root` { width: 100vw; height: 100vh; } which
can cause horizontal scroll due to scrollbar width; change `#root` to use width:
100% and keep height behavior via 100vh or better by setting html and body to
height: 100% and then `#root` to height: 100%; update the stylesheet by replacing
100vw with 100% on the `#root` selector and add/ensure html and body selectors
include height: 100% (and optionally overflow-x: hidden on body if you want to
forcibly prevent horizontal scroll).
- Around line 7-9: The Mapbox GL CSS in index.html (the link element referencing
mapbox-gl.css) conflicts with the npm package version (^3.3.0 in package.json)
and is also duplicated by an import in frontend/src/main.jsx; fix by choosing
one approach: either update the HTML link href to match the installed package
version (v3.3.0) or upgrade the npm dependency to v3.18.1 so both match, and
remove the duplicate CSS load (either delete the CDN <link> in index.html or
remove the CSS import in frontend/src/main.jsx) so the stylesheet is only loaded
once.
In `@frontend/src/components/BookSearch.jsx`:
- Around line 1-47: Add a stale-response guard and clean up debounce timers on
unmount: generate a request counter ref (e.g., latestReqRef) and increment it
inside handleInputChange/doSearch; capture the current id at the start of
doSearch and only call setResults/setError/setLoading if the id matches
latestReqRef.current (this prevents out-of-order responses from updating state).
Also add a useEffect cleanup that clears debounceRef.current timeout and, if you
use AbortController with fetchLibrarianSearch, abort the controller on unmount
(or otherwise mark latestReqRef to a new value) so no pending callbacks run
after unmount; update doSearch to accept/use an optional AbortSignal or check
the request id before setting state. Ensure references to debounceRef, doSearch,
handleInputChange, and latestReqRef are used consistently.
In `@frontend/src/components/MapComponent.jsx`:
- Around line 370-423: The popup HTML is built by interpolating unescaped
user/API data (popupContent -> properties.*, deepContext.historical_context)
then passed to mapboxgl.Popup.setHTML, which allows XSS; fix by sanitizing all
interpolated values (properties.era, properties.title, properties.book,
properties.year, properties.quote, deepContext.historical_context, and any
coords-derived strings) before interpolation — either implement a small
escapeHtml utility and call it for each inserted value, or instead construct the
popup with DOM nodes and use setDOMContent to assign text via textContent (avoid
setHTML with raw strings); update the popup creation (where popupRef.current is
assigned and setHTML is called) to use the chosen safe approach and ensure
accentColor remains derived from a whitelist of expected era values rather than
raw input.
- Around line 286-367: The uploaded-markers event handlers are re-attached on
every uploadedBookLocations update causing duplicate listeners; modify the
useEffect that defines addUploadedLayer to register named handler functions (or
store references) for the "click", "mouseenter", and "mouseleave" events on
"uploaded-markers" and remove them in the useEffect cleanup (use map.off for
"uploaded-markers" with the same handler references) and also remove the
"uploaded-markers" and "uploaded-labels" layers and "uploaded-points" source in
that cleanup; ensure addUploadedLayer still adds layers/sources only once (use
map.isStyleLoaded()/map.once as currently) and that the cleanup also unregisters
the style.load listener if you attach one.
In `@mcp-servers/core/conductor.py`:
- Around line 75-111: Ensure conductor.py validates and normalizes the search
inputs before calling _librarian_search: strip and reject empty query (use the
existing action/query variables) and validate/convert limit to an int with
sensible bounds (e.g., default 10, clamp to 1..100 or similar); return
JsonResponse with status 400 for input validation failures. Narrow the exception
handling around the _librarian_search call: catch ValueError specifically and
return a 400 error with the validation message, and catch other exceptions
(generic) as before returning 502; preserve timing calculations (t0, t_start)
and include the same timeline structure but use the appropriate status/error
codes per error type.
In `@mcp-servers/librarian/views.py`:
- Around line 81-88: The JSON-parsing block uses json.loads(request.body) and
then calls body.get("query") which will raise AttributeError if the parsed body
is not an object; add a type guard after parsing to verify isinstance(body,
dict) (or that body has .get) and return JsonResponse({"error": "JSON object
required"}, status=400) if it is not, so subsequent uses of body.get(...) are
safe; update the code around the json.loads(...) / body variable and the
JsonResponse error paths accordingly.
- Around line 90-94: The request handler currently reads limit =
body.get("limit", 10) and passes it directly to _librarian_search; validate and
bound it before that call: ensure the incoming limit is an integer (coerce
numeric strings or reject non-numeric), enforce a sane range (e.g., min 1, max
100) and clamp or return a 400 Bad Request on invalid input, and then pass the
validated/clamped limit to _librarian_search; update the handler's validation
logic around the limit variable and add explicit error handling for bad
types/ranges.
🧹 Nitpick comments (2)
frontend/src/api/archivistClient.js (1)
5-17: Normalize query and limit before calling the Librarian endpoint.
This prevents accidental empty/whitespace searches and keeps limit in a safe range.🛠️ Suggested fix
export async function fetchLibrarianSearch(query, limit = 10) { + const safeQuery = String(query ?? "").trim(); + if (!safeQuery) throw new Error("Query is required"); + const safeLimit = Number.isFinite(limit) + ? Math.max(1, Math.min(limit, 20)) + : 10; const res = await fetch(`${MCP_BASE_URL}/tools/librarian/search`, { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ query, limit }), + body: JSON.stringify({ query: safeQuery, limit: safeLimit }), });frontend/src/components/MapComponent.jsx (1)
231-240: Avoid returning a value from the forEach callback.
This matches the linter expectation and avoids accidental return values.🛠️ Suggested fix
- eraFeatures.forEach((f) => bounds.extend(f.geometry.coordinates)); + eraFeatures.forEach((f) => { + bounds.extend(f.geometry.coordinates); + });
| <link | ||
| href="https://api.mapbox.com/mapbox-gl-js/v3.3.0/mapbox-gl.css" | ||
| href="https://api.mapbox.com/mapbox-gl-js/v3.18.1/mapbox-gl.css" | ||
| rel="stylesheet" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Mapbox GL JS version in package.json
echo "=== Checking package.json for Mapbox GL version ==="
find . -name "package.json" -type f | head -5 | xargs grep -l "mapbox-gl" 2>/dev/null | head -1 | xargs cat | grep -A 2 -B 2 "mapbox-gl"
echo ""
echo "=== Searching for mapbox-gl imports in codebase ==="
rg "mapbox-gl" -t js -t jsx -t ts -t tsx --max-count 10
echo ""
echo "=== Checking for any Mapbox imports in frontend directory ==="
rg "mapbox" frontend/ --max-count 10 2>/dev/null || echo "No mapbox imports found in frontend/"Repository: thejonathangu/tartanhacks
Length of output: 2563
Fix Mapbox GL CSS/JS version mismatch.
The CSS version in the HTML (v3.18.1 from CDN) does not match the npm package version (^3.3.0 in package.json). The JS code imports from the npm package while the HTML links to the CDN, causing potential rendering conflicts.
Update the HTML link to use v3.3.0 to match the installed package version, or upgrade the npm package to v3.18.1. Additionally, the CSS is being loaded twice (CDN link in index.html and import in frontend/src/main.jsx)—remove the redundant source.
🤖 Prompt for AI Agents
In `@frontend/index.html` around lines 7 - 9, The Mapbox GL CSS in index.html (the
link element referencing mapbox-gl.css) conflicts with the npm package version
(^3.3.0 in package.json) and is also duplicated by an import in
frontend/src/main.jsx; fix by choosing one approach: either update the HTML link
href to match the installed package version (v3.3.0) or upgrade the npm
dependency to v3.18.1 so both match, and remove the duplicate CSS load (either
delete the CDN <link> in index.html or remove the CSS import in
frontend/src/main.jsx) so the stylesheet is only loaded once.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| * { margin: 0; padding: 0; box-sizing: border-box; } | ||
| html, body { width: 100%; height: 100%; overflow: hidden; font-family: system-ui, sans-serif; } | ||
| #root { width: 100%; height: 100%; } | ||
| body { font-family: system-ui, sans-serif; } | ||
| #root { width: 100vw; height: 100vh; } |
There was a problem hiding this comment.
Avoid 100vw width to prevent horizontal scrollbars.
100vw includes scrollbar width and can introduce horizontal overflow; prefer 100% with explicit html/body height.
🧩 Proposed CSS adjustment
- body { font-family: system-ui, sans-serif; }
- `#root` { width: 100vw; height: 100vh; }
+ html, body { height: 100%; }
+ body { font-family: system-ui, sans-serif; }
+ `#root` { width: 100%; height: 100%; }📝 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.
| * { margin: 0; padding: 0; box-sizing: border-box; } | |
| html, body { width: 100%; height: 100%; overflow: hidden; font-family: system-ui, sans-serif; } | |
| #root { width: 100%; height: 100%; } | |
| body { font-family: system-ui, sans-serif; } | |
| #root { width: 100vw; height: 100vh; } | |
| * { margin: 0; padding: 0; box-sizing: border-box; } | |
| html, body { height: 100%; } | |
| body { font-family: system-ui, sans-serif; } | |
| `#root` { width: 100%; height: 100%; } |
🤖 Prompt for AI Agents
In `@frontend/index.html` around lines 12 - 14, The CSS sets `#root` { width: 100vw;
height: 100vh; } which can cause horizontal scroll due to scrollbar width;
change `#root` to use width: 100% and keep height behavior via 100vh or better by
setting html and body to height: 100% and then `#root` to height: 100%; update the
stylesheet by replacing 100vw with 100% on the `#root` selector and add/ensure
html and body selectors include height: 100% (and optionally overflow-x: hidden
on body if you want to forcibly prevent horizontal scroll).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| import React, { useState, useCallback, useRef } from "react"; | ||
| import { fetchLibrarianSearch } from "../api/archivistClient"; | ||
|
|
||
| /** | ||
| * BookSearch — A search widget that queries the LibrarianAgent | ||
| * (Open Library) and renders selectable book results. | ||
| * | ||
| * Props: | ||
| * - onBookSelect(book) — called when the user clicks a book result | ||
| * - accentColor — theme accent (defaults to #4ecdc4) | ||
| */ | ||
| export default function BookSearch({ onBookSelect, accentColor = "#4ecdc4" }) { | ||
| const [query, setQuery] = useState(""); | ||
| const [results, setResults] = useState(null); | ||
| const [loading, setLoading] = useState(false); | ||
| const [error, setError] = useState(null); | ||
| const [selectedKey, setSelectedKey] = useState(null); | ||
| const debounceRef = useRef(null); | ||
|
|
||
| const doSearch = useCallback(async (q) => { | ||
| if (!q.trim()) { | ||
| setResults(null); | ||
| setError(null); | ||
| return; | ||
| } | ||
| setLoading(true); | ||
| setError(null); | ||
| setSelectedKey(null); | ||
| try { | ||
| const data = await fetchLibrarianSearch(q, 8); | ||
| setResults(data); | ||
| } catch (err) { | ||
| console.error("LibrarianAgent search failed:", err); | ||
| setError("Search failed — please try again."); | ||
| setResults(null); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, []); | ||
|
|
||
| const handleInputChange = (e) => { | ||
| const val = e.target.value; | ||
| setQuery(val); | ||
| // Debounce 400ms | ||
| if (debounceRef.current) clearTimeout(debounceRef.current); | ||
| debounceRef.current = setTimeout(() => doSearch(val), 400); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f -name "BookSearch.jsx" --type fRepository: thejonathangu/tartanhacks
Length of output: 299
🏁 Script executed:
find . -name "BookSearch.jsx" -type fRepository: thejonathangu/tartanhacks
Length of output: 109
🏁 Script executed:
cat -n ./frontend/src/components/BookSearch.jsxRepository: thejonathangu/tartanhacks
Length of output: 11221
Prevent stale search results and clear debounce on unmount.
Fast typing can return out-of-order results because concurrent requests may complete in any order without a stale-response guard. Pending debounce timers can also fire after unmount, triggering state updates on an unmounted component.
🛠️ Suggested fix
-import React, { useState, useCallback, useRef } from "react";
+import React, { useState, useCallback, useRef, useEffect } from "react";
@@
const [selectedKey, setSelectedKey] = useState(null);
const debounceRef = useRef(null);
+ const requestSeq = useRef(0);
@@
const doSearch = useCallback(async (q) => {
+ const seq = ++requestSeq.current;
if (!q.trim()) {
+ requestSeq.current += 1;
setResults(null);
setError(null);
return;
}
@@
try {
const data = await fetchLibrarianSearch(q, 8);
+ if (seq !== requestSeq.current) return;
setResults(data);
} catch (err) {
@@
}, []);
+
+ useEffect(() => {
+ return () => {
+ if (debounceRef.current) clearTimeout(debounceRef.current);
+ requestSeq.current += 1;
+ };
+ }, []);🤖 Prompt for AI Agents
In `@frontend/src/components/BookSearch.jsx` around lines 1 - 47, Add a
stale-response guard and clean up debounce timers on unmount: generate a request
counter ref (e.g., latestReqRef) and increment it inside
handleInputChange/doSearch; capture the current id at the start of doSearch and
only call setResults/setError/setLoading if the id matches latestReqRef.current
(this prevents out-of-order responses from updating state). Also add a useEffect
cleanup that clears debounceRef.current timeout and, if you use AbortController
with fetchLibrarianSearch, abort the controller on unmount (or otherwise mark
latestReqRef to a new value) so no pending callbacks run after unmount; update
doSearch to accept/use an optional AbortSignal or check the request id before
setting state. Ensure references to debounceRef, doSearch, handleInputChange,
and latestReqRef are used consistently.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| // ── Uploaded book locations from PDF ── | ||
| useEffect(() => { | ||
| const map = mapRef.current; | ||
| if (!map || !uploadedBookLocations) return; | ||
|
|
||
| const addUploadedLayer = () => { | ||
| // Remove old layers if they exist | ||
| if (map.getLayer("uploaded-markers")) map.removeLayer("uploaded-markers"); | ||
| if (map.getLayer("uploaded-labels")) map.removeLayer("uploaded-labels"); | ||
| if (map.getSource("uploaded-points")) map.removeSource("uploaded-points"); | ||
|
|
||
| map.addSource("uploaded-points", { | ||
| type: "geojson", | ||
| data: uploadedBookLocations, | ||
| }); | ||
|
|
||
| map.addLayer({ | ||
| id: "uploaded-markers", | ||
| type: "circle", | ||
| source: "uploaded-points", | ||
| paint: { | ||
| "circle-radius": 10, | ||
| "circle-color": "#b388ff", | ||
| "circle-stroke-width": 2, | ||
| "circle-stroke-color": "#ffffff", | ||
| "circle-opacity": 0.9, | ||
| }, | ||
| }); | ||
|
|
||
| map.addLayer({ | ||
| id: "uploaded-labels", | ||
| type: "symbol", | ||
| source: "uploaded-points", | ||
| layout: { | ||
| "text-field": ["get", "title"], | ||
| "text-size": 11, | ||
| "text-offset": [0, 1.8], | ||
| "text-anchor": "top", | ||
| "text-max-width": 12, | ||
| }, | ||
| paint: { | ||
| "text-color": "#b388ff", | ||
| "text-halo-color": "#000", | ||
| "text-halo-width": 1, | ||
| }, | ||
| }); | ||
|
|
||
| // Click handlers | ||
| map.on("click", "uploaded-markers", (e) => { | ||
| const feature = e.features[0]; | ||
| const coords = feature.geometry.coordinates.slice(); | ||
| map.flyTo({ center: coords, zoom: 14, speed: 1.2, pitch: 45 }); | ||
| if (onMarkerClickRef.current) onMarkerClickRef.current(feature); | ||
| }); | ||
|
|
||
| map.on("mouseenter", "uploaded-markers", () => { | ||
| map.getCanvas().style.cursor = "pointer"; | ||
| }); | ||
| map.on("mouseleave", "uploaded-markers", () => { | ||
| map.getCanvas().style.cursor = ""; | ||
| }); | ||
|
|
||
| // Fit map to show all uploaded points | ||
| if (uploadedBookLocations.features.length > 0) { | ||
| const bounds = uploadedBookLocations.features.reduce( | ||
| (b, f) => { | ||
| return b.extend(f.geometry.coordinates); | ||
| }, | ||
| new mapboxgl.LngLatBounds( | ||
| uploadedBookLocations.features[0].geometry.coordinates, | ||
| uploadedBookLocations.features[0].geometry.coordinates, | ||
| ), | ||
| ); | ||
| map.fitBounds(bounds, { padding: 80, maxZoom: 12 }); | ||
| } | ||
| }; | ||
|
|
||
| if (map.isStyleLoaded()) { | ||
| addUploadedLayer(); | ||
| } else { | ||
| map.once("style.load", addUploadedLayer); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd frontend/src/components && wc -l MapComponent.jsxRepository: thejonathangu/tartanhacks
Length of output: 89
🏁 Script executed:
cd frontend/src/components && sed -n '280,380p' MapComponent.jsxRepository: thejonathangu/tartanhacks
Length of output: 3224
🌐 Web query:
Mapbox GL JS map.on() event handler duplication stacking behavior
💡 Result:
map.on(type, handler) stacks: every time you call it, Mapbox GL JS registers another listener, so the handler will run multiple times per event if you accidentally attach it repeatedly (for example, inside a React render/effect that re-runs). Mapbox GL JS’ event system is based on Evented (“bind and unbind listeners”) and does not imply automatic de-duplication. [1]
How to avoid / undo duplicates:
-
Detach explicitly with the same function reference (anonymous inline functions can’t be removed because you can’t pass the same reference back):
function onClick(e) { /* ... */ } map.on('click', onClick); // later map.off('click', onClick);
(This is the common fix when you see “double click handler” type bugs.) [2]
-
Use
map.once(...)if you only want it to run one time. [2] -
In component frameworks, attach in an effect and cleanup in the return function (or guard with a flag) so it doesn’t re-register on every re-run. [2]
-
If you’re on newer GL JS, consider the Interactions API (
map.addInteraction(id, ...)) which is keyed by an interaction id, making it easier to manage/replace/remove without accidental stacking. [3]
Sources:
[1] Mapbox GL JS “Events and event types” (Evented interface) https://docs.mapbox.com/mapbox-gl-js/api/events/
[2] Stack Overflow: “Double on click event with mapbox gl” (use once / off and named handlers) https://stackoverflow.com/questions/45663869/double-on-click-event-with-mapbox-gl
[3] Mapbox GL JS Interactions guide (Interactions API) https://docs.mapbox.com/mapbox-gl-js/guides/user-interactions/interactions/
🏁 Script executed:
cd frontend/src/components && rg "map\.on\(" MapComponent.jsx -A 2 -B 2Repository: thejonathangu/tartanhacks
Length of output: 649
🏁 Script executed:
cd frontend/src/components && rg "useEffect.*=>.*\{" MapComponent.jsx -A 5 | head -80Repository: thejonathangu/tartanhacks
Length of output: 1435
🏁 Script executed:
cd frontend/src/components && grep -n "return.*=>" MapComponent.jsx | head -20Repository: thejonathangu/tartanhacks
Length of output: 103
🏁 Script executed:
cd frontend/src/components && sed -n '195,215p' MapComponent.jsxRepository: thejonathangu/tartanhacks
Length of output: 561
Prevent duplicate event handlers on uploaded layers.
Each uploadedBookLocations update re-attaches handlers without cleanup, causing them to stack and trigger repeatedly. Mapbox GL JS event handlers do not auto-deduplicate, so calling map.on() multiple times registers the listener again rather than replacing it. Since the handlers are inline arrow functions, they cannot be removed via map.off() without a stored reference.
Suggested fix
+ const uploadedHandlersRef = useRef(null);
+
useEffect(() => {
const map = mapRef.current;
if (!map || !uploadedBookLocations) return;
+ if (!uploadedHandlersRef.current) {
+ uploadedHandlersRef.current = {
+ onClick: (e) => {
+ const m = mapRef.current;
+ if (!m) return;
+ const feature = e.features[0];
+ const coords = feature.geometry.coordinates.slice();
+ m.flyTo({ center: coords, zoom: 14, speed: 1.2, pitch: 45 });
+ if (onMarkerClickRef.current) onMarkerClickRef.current(feature);
+ },
+ onEnter: () => {
+ const m = mapRef.current;
+ if (m) m.getCanvas().style.cursor = "pointer";
+ },
+ onLeave: () => {
+ const m = mapRef.current;
+ if (m) m.getCanvas().style.cursor = "";
+ },
+ };
+ }
+
const addUploadedLayer = () => {
- map.on("click", "uploaded-markers", (e) => {
- const feature = e.features[0];
- const coords = feature.geometry.coordinates.slice();
- map.flyTo({ center: coords, zoom: 14, speed: 1.2, pitch: 45 });
- if (onMarkerClickRef.current) onMarkerClickRef.current(feature);
- });
-
- map.on("mouseenter", "uploaded-markers", () => {
- map.getCanvas().style.cursor = "pointer";
- });
- map.on("mouseleave", "uploaded-markers", () => {
- map.getCanvas().style.cursor = "";
- });
+ const { onClick, onEnter, onLeave } = uploadedHandlersRef.current;
+ map.off("click", "uploaded-markers", onClick);
+ map.off("mouseenter", "uploaded-markers", onEnter);
+ map.off("mouseleave", "uploaded-markers", onLeave);
+ map.on("click", "uploaded-markers", onClick);
+ map.on("mouseenter", "uploaded-markers", onEnter);
+ map.on("mouseleave", "uploaded-markers", onLeave);🤖 Prompt for AI Agents
In `@frontend/src/components/MapComponent.jsx` around lines 286 - 367, The
uploaded-markers event handlers are re-attached on every uploadedBookLocations
update causing duplicate listeners; modify the useEffect that defines
addUploadedLayer to register named handler functions (or store references) for
the "click", "mouseenter", and "mouseleave" events on "uploaded-markers" and
remove them in the useEffect cleanup (use map.off for "uploaded-markers" with
the same handler references) and also remove the "uploaded-markers" and
"uploaded-labels" layers and "uploaded-points" source in that cleanup; ensure
addUploadedLayer still adds layers/sources only once (use
map.isStyleLoaded()/map.once as currently) and that the cleanup also unregisters
the style.load listener if you attach one.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| // ── Popup rendering ── | ||
| useEffect(() => { | ||
| if (!popupContent || !mapRef.current) return; | ||
| if (popupRef.current) popupRef.current.remove(); | ||
|
|
||
| const { geometry, properties, deepContext } = popupContent; | ||
| const coords = geometry.coordinates; | ||
| const [lng, lat] = coords; | ||
|
|
||
| const accentColor = | ||
| properties.era === "1940s" | ||
| ? "#e6b800" | ||
| : properties.era === "1920s" | ||
| ? "#ff6b6b" | ||
| : "#4ecdc4"; | ||
|
|
||
| const streetViewUrl = `https://maps.googleapis.com/maps/api/streetview?size=600x300&location=${lat},${lng}&fov=90&heading=235&pitch=10&key=${GOOGLE_API_KEY}`; | ||
|
|
||
| const html = ` | ||
| <div style="max-width:320px;font-family:system-ui,sans-serif;color:#1a1a2e;"> | ||
| <div style="position:relative;margin:-10px -10px 8px -10px;overflow:hidden;border-radius:8px 8px 0 0;"> | ||
| <img src="${streetViewUrl}" alt="Street View" | ||
| style="width:100%;height:160px;object-fit:cover;display:block;" | ||
| onerror="this.parentElement.style.display='none'" /> | ||
| <div style="position:absolute;top:8px;right:8px;background:${accentColor}; | ||
| padding:2px 8px;border-radius:12px;font-size:10px;font-weight:700;color:#000;"> | ||
| ${properties.era} | ||
| </div> | ||
| </div> | ||
| <h3 style="margin:0 0 4px;font-size:14px;">${properties.title}</h3> | ||
| <p style="color:#888;margin:0 0 8px;font-size:11px;">${properties.book} · ${properties.year}</p> | ||
| <blockquote style="border-left:3px solid ${accentColor};padding-left:8px;margin:0 0 8px; | ||
| font-style:italic;font-size:12px;color:#444;line-height:1.5;"> | ||
| "${properties.quote}" | ||
| </blockquote> | ||
| ${ | ||
| deepContext?.historical_context | ||
| ? ` | ||
| <p style="font-size:11px;color:#555;margin:0;line-height:1.4;"> | ||
| <strong style="color:${accentColor};">🏛</strong> ${deepContext.historical_context} | ||
| </p> | ||
| ` | ||
| : `<p style="font-size:11px;color:#999;margin:0;">Click explores this via MCP agents →</p>` | ||
| } | ||
| </div> | ||
| `; | ||
|
|
||
| popupRef.current = new mapboxgl.Popup({ | ||
| closeOnClick: true, | ||
| maxWidth: "340px", | ||
| }) | ||
| .setLngLat(coords) | ||
| .setHTML(html) | ||
| .addTo(mapRef.current); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "MapComponent.jsx" -type fRepository: thejonathangu/tartanhacks
Length of output: 111
🏁 Script executed:
sed -n '370,423p' ./frontend/src/components/MapComponent.jsx | cat -nRepository: thejonathangu/tartanhacks
Length of output: 2744
🏁 Script executed:
grep -n "setPopupContent\|popupContent" ./frontend/src/components/MapComponent.jsx | head -20Repository: thejonathangu/tartanhacks
Length of output: 238
🏁 Script executed:
sed -n '1,100p' ./frontend/src/components/MapComponent.jsx | cat -nRepository: thejonathangu/tartanhacks
Length of output: 3727
🏁 Script executed:
grep -r "popupContent" ./frontend/src --include="*.jsx" --include="*.js" -n | grep -v "MapComponent.jsx"Repository: thejonathangu/tartanhacks
Length of output: 219
🏁 Script executed:
sed -n '120,200p' ./frontend/src/App.jsx | cat -nRepository: thejonathangu/tartanhacks
Length of output: 3286
🏁 Script executed:
grep -n "fetchConductorOrchestrate" ./frontend/src/App.jsx | head -5Repository: thejonathangu/tartanhacks
Length of output: 300
🏁 Script executed:
grep -n "literaryGeoJSON" ./frontend/src/components/MapComponent.jsxRepository: thejonathangu/tartanhacks
Length of output: 278
🏁 Script executed:
find ./frontend/src/data -name "literaryPoints*" -type fRepository: thejonathangu/tartanhacks
Length of output: 106
🏁 Script executed:
head -100 ./frontend/src/data/literaryPoints.js | cat -nRepository: thejonathangu/tartanhacks
Length of output: 4506
🏁 Script executed:
grep -n "uploadedBookLocations" ./frontend/src/App.jsx | head -10Repository: thejonathangu/tartanhacks
Length of output: 209
🏁 Script executed:
sed -n '135,250p' ./frontend/src/App.jsx | cat -nRepository: thejonathangu/tartanhacks
Length of output: 4700
🏁 Script executed:
grep -rn "setUploadedBookLocations" ./frontend/src --include="*.jsx" --include="*.js"Repository: thejonathangu/tartanhacks
Length of output: 261
🏁 Script executed:
sed -n '450,470p' ./frontend/src/App.jsx | cat -nRepository: thejonathangu/tartanhacks
Length of output: 860
🏁 Script executed:
find ./frontend/src -name "BookUpload*" -type fRepository: thejonathangu/tartanhacks
Length of output: 109
🏁 Script executed:
grep -n "onLocationsExtracted" ./frontend/src/components/BookUpload.jsx | head -10Repository: thejonathangu/tartanhacks
Length of output: 206
Sanitize popup HTML content to prevent XSS.
The code interpolates user data directly into HTML via setHTML(): API responses (deepContext.historical_context) and uploaded book data can inject malicious scripts. Apply HTML escaping to all interpolated values—including properties.era on line 27, which is also unescaped.
🛠️ Suggested fix (escape values before interpolation)
+const escapeHtml = (value = "") =>
+ String(value).replace(/[&<>"']/g, (c) => ({
+ "&": "&",
+ "<": "<",
+ ">": ">",
+ '"': """,
+ "'": "'",
+ }[c]));
@@
- const html = `
+ const html = `
<div style="max-width:320px;font-family:system-ui,sans-serif;color:`#1a1a2e`;">
@@
+ ${escapeHtml(properties.era)}
@@
- <h3 style="margin:0 0 4px;font-size:14px;">${properties.title}</h3>
- <p style="color:`#888`;margin:0 0 8px;font-size:11px;">${properties.book} · ${properties.year}</p>
+ <h3 style="margin:0 0 4px;font-size:14px;">${escapeHtml(properties.title)}</h3>
+ <p style="color:`#888`;margin:0 0 8px;font-size:11px;">${escapeHtml(properties.book)} · ${escapeHtml(properties.year)}</p>
@@
- "${properties.quote}"
+ "${escapeHtml(properties.quote)}"
@@
- <strong style="color:${accentColor};">🏛</strong> ${deepContext.historical_context}
+ <strong style="color:${accentColor};">🏛</strong> ${escapeHtml(deepContext.historical_context)}🤖 Prompt for AI Agents
In `@frontend/src/components/MapComponent.jsx` around lines 370 - 423, The popup
HTML is built by interpolating unescaped user/API data (popupContent ->
properties.*, deepContext.historical_context) then passed to
mapboxgl.Popup.setHTML, which allows XSS; fix by sanitizing all interpolated
values (properties.era, properties.title, properties.book, properties.year,
properties.quote, deepContext.historical_context, and any coords-derived
strings) before interpolation — either implement a small escapeHtml utility and
call it for each inserted value, or instead construct the popup with DOM nodes
and use setDOMContent to assign text via textContent (avoid setHTML with raw
strings); update the popup creation (where popupRef.current is assigned and
setHTML is called) to use the chosen safe approach and ensure accentColor
remains derived from a whitelist of expected era values rather than raw input.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| action = body.get("action") | ||
|
|
||
| # ── Book search shortcut — delegates to LibrarianAgent only ── | ||
| if action == "search": | ||
| query = body.get("query") | ||
| if not query: | ||
| return JsonResponse({"error": "query is required for search action"}, status=400) | ||
| limit = body.get("limit", 10) | ||
| t0 = time.perf_counter() | ||
| try: | ||
| result = _librarian_search(query, limit=limit) | ||
| elapsed = round((time.perf_counter() - t0) * 1000) | ||
| total = round((time.perf_counter() - t_start) * 1000) | ||
| return JsonResponse({ | ||
| "librarian": result, | ||
| "timeline": [{ | ||
| "agent": "LibrarianAgent", | ||
| "tool": "search_books", | ||
| "status": "success", | ||
| "elapsed_ms": elapsed, | ||
| }], | ||
| "total_ms": total, | ||
| }) | ||
| except Exception as exc: | ||
| elapsed = round((time.perf_counter() - t0) * 1000) | ||
| total = round((time.perf_counter() - t_start) * 1000) | ||
| return JsonResponse({ | ||
| "error": str(exc), | ||
| "timeline": [{ | ||
| "agent": "LibrarianAgent", | ||
| "tool": "search_books", | ||
| "status": "error", | ||
| "elapsed_ms": elapsed, | ||
| "error": str(exc), | ||
| }], | ||
| "total_ms": total, | ||
| }, status=502) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "conductor.py" -type f | head -20Repository: thejonathangu/tartanhacks
Length of output: 100
🏁 Script executed:
git ls-files | grep -E "(conductor|librarian)" | head -20Repository: thejonathangu/tartanhacks
Length of output: 223
🏁 Script executed:
wc -l ./mcp-servers/core/conductor.pyRepository: thejonathangu/tartanhacks
Length of output: 104
🏁 Script executed:
cat -n ./mcp-servers/core/conductor.py | head -150Repository: thejonathangu/tartanhacks
Length of output: 6703
🏁 Script executed:
cat -n ./mcp-servers/librarian/views.pyRepository: thejonathangu/tartanhacks
Length of output: 4072
🏁 Script executed:
grep -n "import" ./mcp-servers/core/conductor.pyRepository: thejonathangu/tartanhacks
Length of output: 622
Validate search inputs and narrow exception handling.
The _librarian_search function validates and strips the query internally, but conductor.py should validate inputs earlier. More critically, limit is never validated—it can be a string, float, or out of bounds, causing unexpected behavior downstream. The blanket Exception catch also masks the distinction between validation errors (which should return 400) and network/API errors (502): _librarian_search raises ValueError for invalid queries, which should map to 400, not 502.
🛠️ Suggested fix
+import httpx
@@
- if action == "search":
- query = body.get("query")
- if not query:
- return JsonResponse({"error": "query is required for search action"}, status=400)
- limit = body.get("limit", 10)
+ if action == "search":
+ query = (body.get("query") or "").strip()
+ if not query:
+ return JsonResponse({"error": "query is required for search action"}, status=400)
+ try:
+ limit = int(body.get("limit", 10))
+ except (TypeError, ValueError):
+ return JsonResponse({"error": "limit must be an integer"}, status=400)
+ limit = max(1, min(limit, 25))
@@
- try:
- result = _librarian_search(query, limit=limit)
+ try:
+ result = _librarian_search(query, limit=limit)
@@
- except Exception as exc:
+ except ValueError as exc:
+ elapsed = round((time.perf_counter() - t0) * 1000)
+ total = round((time.perf_counter() - t_start) * 1000)
+ return JsonResponse({
+ "error": str(exc),
+ "timeline": [{
+ "agent": "LibrarianAgent",
+ "tool": "search_books",
+ "status": "error",
+ "elapsed_ms": elapsed,
+ "error": str(exc),
+ }],
+ "total_ms": total,
+ }, status=400)
+ except (httpx.HTTPStatusError, httpx.RequestError) as exc:
elapsed = round((time.perf_counter() - t0) * 1000)
total = round((time.perf_counter() - t_start) * 1000)
return JsonResponse({
"error": str(exc),
"timeline": [{
"agent": "LibrarianAgent",
"tool": "search_books",
"status": "error",
"elapsed_ms": elapsed,
"error": str(exc),
}],
"total_ms": total,
}, status=502)🧰 Tools
🪛 Ruff (0.14.14)
[warning] 98-98: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@mcp-servers/core/conductor.py` around lines 75 - 111, Ensure conductor.py
validates and normalizes the search inputs before calling _librarian_search:
strip and reject empty query (use the existing action/query variables) and
validate/convert limit to an int with sensible bounds (e.g., default 10, clamp
to 1..100 or similar); return JsonResponse with status 400 for input validation
failures. Narrow the exception handling around the _librarian_search call: catch
ValueError specifically and return a 400 error with the validation message, and
catch other exceptions (generic) as before returning 502; preserve timing
calculations (t0, t_start) and include the same timeline structure but use the
appropriate status/error codes per error type.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| try: | ||
| body = json.loads(request.body) | ||
| except json.JSONDecodeError: | ||
| return JsonResponse({"error": "Invalid JSON"}, status=400) | ||
|
|
||
| query = body.get("query") | ||
| if not query: | ||
| return JsonResponse({"error": "query is required"}, status=400) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the actual file
find . -type f -name "views.py" | grep -i librarianRepository: thejonathangu/tartanhacks
Length of output: 101
🏁 Script executed:
# Understand Python's json.loads() behavior with different JSON types
python3 << 'EOF'
import json
# Test 1: JSON array
array_json = '["item1", "item2"]'
array_result = json.loads(array_json)
print(f"Array JSON result type: {type(array_result)}")
print(f"Array JSON result: {array_result}")
# Test 2: JSON string
string_json = '"hello"'
string_result = json.loads(string_json)
print(f"String JSON result type: {type(string_result)}")
print(f"String JSON result: {string_result}")
# Test 3: JSON number
number_json = '123'
number_result = json.loads(number_json)
print(f"Number JSON result type: {type(number_result)}")
print(f"Number JSON result: {number_result}")
# Test 4: JSON null
null_json = 'null'
null_result = json.loads(null_json)
print(f"Null JSON result type: {type(null_result)}")
print(f"Null JSON result: {null_result}")
# Test 5: What happens when calling .get() on non-dict types?
print("\n--- Testing .get() on non-dict types ---")
test_cases = [array_result, string_result, number_result, null_result]
for i, test_value in enumerate(test_cases):
try:
result = test_value.get("query")
print(f"Test {i+1} (.get() succeeded): {result}")
except AttributeError as e:
print(f"Test {i+1} (.get() failed with AttributeError): {type(test_value).__name__} has no attribute 'get'")
except Exception as e:
print(f"Test {i+1} (.get() failed with {type(e).__name__}): {e}")
EOFRepository: thejonathangu/tartanhacks
Length of output: 670
🏁 Script executed:
cat -n ./mcp-servers/librarian/views.pyRepository: thejonathangu/tartanhacks
Length of output: 4072
Add type guard to ensure JSON body is an object.
If the request payload is a valid JSON array, string, number, or null, body.get(...) on line 86 (and line 90) will raise an uncaught AttributeError, resulting in a 500 error instead of the intended 400.
🛡️ Suggested guard
try:
body = json.loads(request.body)
except json.JSONDecodeError:
return JsonResponse({"error": "Invalid JSON"}, status=400)
+ if not isinstance(body, dict):
+ return JsonResponse({"error": "JSON object required"}, status=400)
+
query = body.get("query")📝 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.
| try: | |
| body = json.loads(request.body) | |
| except json.JSONDecodeError: | |
| return JsonResponse({"error": "Invalid JSON"}, status=400) | |
| query = body.get("query") | |
| if not query: | |
| return JsonResponse({"error": "query is required"}, status=400) | |
| try: | |
| body = json.loads(request.body) | |
| except json.JSONDecodeError: | |
| return JsonResponse({"error": "Invalid JSON"}, status=400) | |
| if not isinstance(body, dict): | |
| return JsonResponse({"error": "JSON object required"}, status=400) | |
| query = body.get("query") | |
| if not query: | |
| return JsonResponse({"error": "query is required"}, status=400) |
🤖 Prompt for AI Agents
In `@mcp-servers/librarian/views.py` around lines 81 - 88, The JSON-parsing block
uses json.loads(request.body) and then calls body.get("query") which will raise
AttributeError if the parsed body is not an object; add a type guard after
parsing to verify isinstance(body, dict) (or that body has .get) and return
JsonResponse({"error": "JSON object required"}, status=400) if it is not, so
subsequent uses of body.get(...) are safe; update the code around the
json.loads(...) / body variable and the JsonResponse error paths accordingly.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| limit = body.get("limit", 10) | ||
|
|
||
| try: | ||
| result = _librarian_search(query, limit=limit) | ||
| except ValueError as e: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check its size
find . -name "views.py" -path "*/librarian/*" | head -5Repository: thejonathangu/tartanhacks
Length of output: 101
🏁 Script executed:
# Read the file around lines 90-94 to understand the context
wc -l mcp-servers/librarian/views.pyRepository: thejonathangu/tartanhacks
Length of output: 103
🏁 Script executed:
# Get the relevant section with context
sed -n '80,105p' mcp-servers/librarian/views.pyRepository: thejonathangu/tartanhacks
Length of output: 875
🏁 Script executed:
# Find the _librarian_search function definition
grep -n "_librarian_search" mcp-servers/librarian/views.pyRepository: thejonathangu/tartanhacks
Length of output: 189
🏁 Script executed:
# Read the function definition
sed -n '1,50p' mcp-servers/librarian/views.pyRepository: thejonathangu/tartanhacks
Length of output: 1522
🏁 Script executed:
# Check the full _librarian_search implementation
sed -n '24,70p' mcp-servers/librarian/views.pyRepository: thejonathangu/tartanhacks
Length of output: 1716
🏁 Script executed:
# Search for any validation or limit bounds in the codebase
rg "limit" mcp-servers/librarian/ -A 2 -B 2Repository: thejonathangu/tartanhacks
Length of output: 1467
🏁 Script executed:
# Check if there are any other files that might document Open Library API limits
find . -type f -name "*.md" -o -name "*.txt" -o -name "*.py" | xargs grep -l "Open Library" 2>/dev/null | head -10Repository: thejonathangu/tartanhacks
Length of output: 129
🏁 Script executed:
# Check for any constants or configuration related to limits
rg "limit.*[0-9]+" mcp-servers/librarian/ -B 2 -A 2Repository: thejonathangu/tartanhacks
Length of output: 909
🏁 Script executed:
# Check the entire file to see if there's documentation about API constraints
cat -n mcp-servers/librarian/views.pyRepository: thejonathangu/tartanhacks
Length of output: 4072
🌐 Web query:
Open Library API search limit parameter bounds constraints
💡 Result:
For Open Library’s Search API (GET https://openlibrary.org/search.json), the docs describe limit only as a pagination page-size parameter (usable with either offset or page) but do not document any minimum/maximum bounds or a hard cap for limit. [1]
Related Open Library endpoints do document caps (useful as a reference point, but they are different APIs), e.g. the Query API (/query.json) explicitly caps limit at 1000 “for performance reasons”. [2]
Sources:
[1] Open Library Developer Center — Search API (offset/limit, page/limit, no max stated) (openlibrary.org)
[2] Open Library Developer Center — RESTful API / Query API (limit max 1000) (openlibrary.org)
Citations:
- 1: https://openlibrary.org/dev/docs/api/search
- 2: https://openlibrary.org/dev/docs/restful_api?utm_source=openai
Validate and bound limit before calling Open Library.
The limit parameter is extracted from the JSON body without type checking or bounds validation. A client can send a non-integer (e.g., "limit": "abc") or an unbounded value (e.g., "limit": 10000000), causing type errors or excessive upstream requests. Add validation to ensure limit is an integer within a reasonable range.
🚧 Suggested validation
- limit = body.get("limit", 10)
+ limit = body.get("limit", 10)
+ try:
+ limit = int(limit)
+ except (TypeError, ValueError):
+ return JsonResponse({"error": "limit must be an integer"}, status=400)
+ if not (1 <= limit <= 50):
+ return JsonResponse({"error": "limit must be between 1 and 50"}, status=400)📝 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.
| limit = body.get("limit", 10) | |
| try: | |
| result = _librarian_search(query, limit=limit) | |
| except ValueError as e: | |
| limit = body.get("limit", 10) | |
| try: | |
| limit = int(limit) | |
| except (TypeError, ValueError): | |
| return JsonResponse({"error": "limit must be an integer"}, status=400) | |
| if not (1 <= limit <= 50): | |
| return JsonResponse({"error": "limit must be between 1 and 50"}, status=400) | |
| try: | |
| result = _librarian_search(query, limit=limit) | |
| except ValueError as e: |
🤖 Prompt for AI Agents
In `@mcp-servers/librarian/views.py` around lines 90 - 94, The request handler
currently reads limit = body.get("limit", 10) and passes it directly to
_librarian_search; validate and bound it before that call: ensure the incoming
limit is an integer (coerce numeric strings or reject non-numeric), enforce a
sane range (e.g., min 1, max 100) and clamp or return a 400 Bad Request on
invalid input, and then pass the validated/clamped limit to _librarian_search;
update the handler's validation logic around the limit variable and add explicit
error handling for bad types/ranges.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@thejonathangu I've opened a new pull request, #19, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #20, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #21, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #22, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #23, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #24, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #25, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #26, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #27, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #28, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #29, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #30, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #31, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #32, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #33, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@thejonathangu I've opened a new pull request, #34, to work on those changes. Once the pull request is ready, I'll request review from you. |
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements