Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Allow delete database schema
WalkthroughCentralizes graph_id namespacing/validation in backend, adds DELETE /{graph_id} endpoint, expands log sanitization, and refactors multiple routes to use the helper. Frontend replaces the select-based graph chooser with a custom dropdown, adds supporting CSS/TS utilities, and updates app/chat/messages logic to use the new graph selection API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant FE as Frontend (graphs.ts / graph_select.ts)
participant API as API /graphs
participant DB as DB/Store
U->>FE: Click "Delete" on graph option
FE->>API: DELETE /graphs/{graph_id}
API->>API: Namespace & validate graph_id
API->>DB: select_graph(namespaced)
alt Graph found
DB-->>API: Graph handle
API->>DB: await graph.delete()
DB-->>API: Deleted
API-->>FE: 200 {"success": true, "graph": graph_id}
FE-->>U: Show success, reload graphs
else Not found
DB-->>API: ResponseError
API-->>FE: 404 {"error":"Failed to delete graph, Graph not found"}
FE-->>U: Show not found
else Other error
API-->>FE: 500 {"error":"Failed to delete graph"}
FE-->>U: Show error
end
sequenceDiagram
autonumber
actor U as User
participant UI as chat_header.j2 (Dropdown UI)
participant GS as graph_select.ts
participant G as graphs.ts
participant MSG as messages.ts
U->>UI: Click selected area
UI->>GS: toggleOptions()
U->>UI: Click a graph option
UI->>GS: onSelect(name)
GS->>G: callback initChat(name)
G->>GS: setSelectedGraph(name)
G->>MSG: initChat() (uses getSelectedGraph)
MSG-->>U: Greeting based on selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
api/routes/graphs.py (1)
234-261: Use _graph_name() here too for consistency and validation.Avoid hand-rolling user_id prefixes; centralize length/trim checks.
- graph_id = f"{request.state.user_id}_{data.database}" + graph_id = _graph_name(request, data.database) success, result = await JSONLoader.load(graph_id, data.dict()) @@ - graph_id = f"{request.state.user_id}_{data.get('database', '')}" + graph_id = _graph_name(request, data.get('database', '')) success, result = await JSONLoader.load(graph_id, data) @@ - graph_id = f"{request.state.user_id}_{filename.replace('.xml', '')}" + graph_id = _graph_name(request, filename.replace('.xml', '')) success, result = await ODataLoader.load(graph_id, xml_data) @@ - graph_id = f"{request.state.user_id}_{filename.replace('.csv', '')}" + graph_id = _graph_name(request, filename.replace('.csv', '')) success, result = await CSVLoader.load(graph_id, csv_data)app/ts/app.ts (2)
25-48: Fix type mismatch for selected graph.getSelectedGraph() returns string | null, but loadAndShowGraph expects string | undefined. Align the signature to avoid TS errors in strict mode.
-async function loadAndShowGraph(selected: string | undefined) { +async function loadAndShowGraph(selected: string | null) { if (!selected) return;
88-96: Avoid double-init and switch to an explicit “graph-selected” event.Clicking an option triggers both the option handler (initChat via onSelect) and this container listener, causing duplicate init/reset behavior. Listen for a dedicated CustomEvent dispatched on selection instead.
Apply:
- // Legacy select is hidden; custom UI will trigger load via graph_select helper - document.getElementById('graph-options')?.addEventListener('click', async () => { - onGraphChange(); - const selected = getSelectedGraph(); - if (!selected) return; - if (DOM.schemaContainer && DOM.schemaContainer.classList.contains('open')) { - await loadAndShowGraph(selected); - } - }); + // React to explicit selection event from the custom dropdown + document.addEventListener('graph-selected', async (e: Event) => { + onGraphChange(); + const selected = (e as CustomEvent<string>).detail ?? getSelectedGraph(); + if (!selected) return; + if (DOM.schemaContainer && DOM.schemaContainer.classList.contains('open')) { + await loadAndShowGraph(selected); + } + });Follow-up change needed in modules/graph_select.ts or graphs.ts to dispatch this event (see comment there).
app/ts/modules/graphs.ts (1)
170-180: Refresh graph list and notify on successful upload.Currently you only log success. Reload to surface the new graph and inform the user.
}).then(response => response.json()) .then(data => { - console.log('File uploaded successfully', data); + addMessage('Schema uploaded successfully. Loading databases…', false); + loadGraphs(); }).catch(error => {
🧹 Nitpick comments (13)
app/public/css/menu.css (2)
336-343: Make the dropdown responsive; avoid fixed width.Use min/max/clamp to prevent overflow on narrow viewports.
.graph-custom-dropdown { position: relative; display: inline-block; - width: 180px; + width: auto; + min-width: 180px; + max-width: 100%; margin-left: 8px; }
396-412: Improve delete button accessibility and theming.
- Show on keyboard focus, not just hover.
- Use theme var for color.
-.graph-options .dropdown-option .delete-btn { +.graph-options .dropdown-option .delete-btn { background: transparent; border: none; - color: #ff6b6b; + color: var(--danger-foreground, #ff6b6b); opacity: 0; cursor: pointer; width: 28px; height: 28px; display: flex; align-items: center; justify-content: center; margin-left: auto; } -.graph-options .dropdown-option:hover .delete-btn { +.graph-options .dropdown-option:hover .delete-btn, +.graph-options .dropdown-option .delete-btn:focus-visible { opacity: 1; + outline: 2px solid var(--falkor-primary); + outline-offset: 2px; }app/ts/modules/graph_select.ts (4)
5-5: Remove unused import.DOM isn’t used in this module.
-import { DOM } from './config';
14-18: Set title for truncation tooltip.Small UX win; keeps visible text truncated but full value discoverable.
export function setSelectedGraph(name: string) { const selectedLabel = document.getElementById('graph-selected'); const textNode = selectedLabel?.querySelector('.dropdown-text'); - if (textNode) textNode.textContent = name; + if (textNode) { + (textNode as HTMLElement).textContent = name; + (textNode as HTMLElement).setAttribute('title', name); + } }
20-23: Avoid innerHTML for clearing.Use node removal to dodge DOM clobbering warnings.
export function clearGraphOptions() { const optionsContainer = document.getElementById('graph-options'); - if (optionsContainer) optionsContainer.innerHTML = ''; + if (optionsContainer) { + while (optionsContainer.firstChild) { + optionsContainer.removeChild(optionsContainer.firstChild); + } + } }
59-62: Reflect open/closed state via ARIA.Improves a11y for screen readers.
export function toggleOptions() { const optionsContainer = document.getElementById('graph-options'); - if (optionsContainer) optionsContainer.classList.toggle('open'); + if (optionsContainer) { + const isOpen = optionsContainer.classList.toggle('open'); + const selectedLabel = document.getElementById('graph-selected'); + selectedLabel?.setAttribute('aria-expanded', isOpen ? 'true' : 'false'); + } }api/routes/graphs.py (2)
88-96: Docstring doesn’t match implementation.It mentions repr(), but the function doesn’t wrap values in repr.
- """ - Sanitize input for safe logging—remove newlines, - carriage returns, tabs, and wrap in repr(). - """ + """ + Sanitize input for safe logging — remove newlines, carriage returns and tabs. + """
101-106: Clarify validation message and allow 200 chars.Message says “less than 200” but code allows 200.
- if not graph_id: - raise HTTPException(status_code=400, - detail="Invalid graph_id, must be less than 200 characters.") + if not graph_id: + raise HTTPException( + status_code=400, + detail="Invalid graph_id, must be at most 200 characters." + )app/ts/modules/messages.ts (1)
141-146: Guard against placeholder selections.If the label shows a placeholder, init should display the guidance branch.
This will be addressed if you adopt the getSelectedGraph() change to ignore placeholders. Please verify that #graph-selected has data-placeholder set in chat_header.j2.
app/ts/modules/chat.ts (2)
13-17: Trim and validate selection before sending.Minor: trim to avoid “ ” being treated as a value.
- const selectedValue = getSelectedGraph() || ''; + const selectedValue = (getSelectedGraph() || '').trim();
261-263: Same trim in confirmation flow for consistency.- const selectedValue = getSelectedGraph() || ''; + const selectedValue = (getSelectedGraph() || '').trim();app/ts/modules/graphs.ts (2)
35-35: Remove noisy console log or gate behind DEBUG.This will spam consoles in production.
- console.log('Graphs loaded:', data); + // console.debug('Graphs loaded:', data);
57-63: Delete legacy option creation (dead code).These options are never appended; keeping them is confusing.
- // populate hidden select for legacy code - data.forEach(graph => { - const option = document.createElement('option'); - option.value = graph; - option.textContent = graph; - option.title = graph; - });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
api/routes/graphs.py(7 hunks)app/public/css/menu.css(1 hunks)app/templates/components/chat_header.j2(1 hunks)app/ts/app.ts(3 hunks)app/ts/modules/chat.ts(2 hunks)app/ts/modules/config.ts(0 hunks)app/ts/modules/graph_select.ts(1 hunks)app/ts/modules/graphs.ts(3 hunks)app/ts/modules/messages.ts(2 hunks)
💤 Files with no reviewable changes (1)
- app/ts/modules/config.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the TypeScript frontend sources in
app/and build them before production runs
Files:
app/ts/modules/graph_select.tsapp/ts/modules/messages.tsapp/ts/modules/chat.tsapp/public/css/menu.cssapp/templates/components/chat_header.j2app/ts/app.tsapp/ts/modules/graphs.ts
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/routes/graphs.py
🧬 Code graph analysis (5)
app/ts/modules/messages.ts (1)
app/ts/modules/graph_select.ts (1)
getSelectedGraph(7-12)
app/ts/modules/chat.ts (2)
app/ts/modules/config.ts (1)
DOM(39-64)app/ts/modules/graph_select.ts (1)
getSelectedGraph(7-12)
api/routes/graphs.py (1)
api/auth/user_management.py (1)
token_required(254-294)
app/ts/app.ts (2)
app/ts/modules/graph_select.ts (1)
getSelectedGraph(7-12)app/ts/modules/graphs.ts (1)
onGraphChange(182-184)
app/ts/modules/graphs.ts (2)
app/ts/modules/graph_select.ts (5)
clearGraphOptions(20-23)addGraphOption(25-57)setSelectedGraph(14-18)toggleOptions(59-62)getSelectedGraph(7-12)app/ts/modules/messages.ts (2)
addMessage(8-75)initChat(134-150)
🪛 ast-grep (0.38.6)
app/ts/modules/graph_select.ts
[warning] 41-41: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: delBtn.innerHTML = <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="3 6 5 6 21 6"></polyline><path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"></path><path d="M10 11v6"></path><path d="M14 11v6"></path><path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"></path></svg>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 21-21: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: optionsContainer.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 41-41: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: delBtn.innerHTML = <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="3 6 5 6 21 6"></polyline><path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"></path><path d="M10 11v6"></path><path d="M14 11v6"></path><path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"></path></svg>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 GitHub Actions: Pylint
api/routes/graphs.py
[error] 75-75: R1705: Unnecessary "elif" after "return" (no-else-return)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (10)
app/public/css/menu.css (1)
419-421: LGTM: open state toggle works with display switching.api/routes/graphs.py (3)
131-136: LGTM: centralized namespacing via _graph_name.This removes duplication and ensures uniform validation.
723-746: DELETE endpoint: good, but ensure the import fix above is applied.With the correct import, 404 handling for missing graphs will work as intended.
After applying the import fix, hit the endpoint on a non-existent graph and expect 404 JSON: {"error":"Failed to delete graph, Graph not found"}.
75-82: Simplify conditional to satisfy Pylint R1705: no-else-return
Replaceelif/elsewith early returns:- if db_url_lower.startswith('postgresql://') or db_url_lower.startswith('postgres://'): - return 'postgresql', PostgresLoader - elif db_url_lower.startswith('mysql://'): - return 'mysql', MySQLLoader - else: - # Default to PostgresLoader for backward compatibility - return 'postgresql', PostgresLoader + if db_url_lower.startswith('postgresql://') or db_url_lower.startswith('postgres://'): + return 'postgresql', PostgresLoader + if db_url_lower.startswith('mysql://'): + return 'mysql', MySQLLoader + # Default to PostgresLoader for backward compatibility + return 'postgresql', PostgresLoaderManually verify by running
pipenv run pylint api/routes/graphs.pyto confirm R1705 is no longer reported.app/ts/modules/messages.ts (1)
6-6: LGTM: centralized selection via helper.app/ts/modules/chat.ts (1)
7-7: LGTM: import helper to unify graph selection.app/templates/components/chat_header.j2 (1)
15-15: Confirm stylesheet inclusion path.Since styles moved to app/public/css/menu.css, verify the layout template includes this file on pages using this component, otherwise the dropdown will be unstyled.
app/ts/app.ts (1)
66-72: Guard uses of selected and rely on the unified type.After updating the signature above, this block is fine. Just a note: selected may be null; current checks already cover it.
app/ts/modules/graphs.ts (2)
106-111: Good defensive re-binding of delete handler.removeEventListener before add avoids duplicates.
182-184: Keep onGraphChange minimal.This is fine as the single entry to reset chat when selection changes.
| from fastapi import APIRouter, Request, HTTPException, UploadFile, File | ||
| from fastapi.responses import JSONResponse, StreamingResponse | ||
| from pydantic import BaseModel | ||
| from redis import ResponseError |
There was a problem hiding this comment.
Incorrect ResponseError import.
redis-py exposes ResponseError under redis.exceptions.
-from redis import ResponseError
+from redis.exceptions import ResponseError📝 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.
| from redis import ResponseError | |
| from redis.exceptions import ResponseError |
🤖 Prompt for AI Agents
In api/routes/graphs.py around line 11 the code imports ResponseError from the
top-level redis module which is incorrect; update the import to use
redis.exceptions.ResponseError (i.e., replace the current import with one that
imports ResponseError from redis.exceptions) so the correct exception class from
redis-py is referenced.
| .graph-options { | ||
| position: absolute; | ||
| top: calc(100%); | ||
| left: 0; | ||
| right: 0; | ||
| background: var(--falkor-secondary); | ||
| border: 1px solid var(--border-color); | ||
| border-radius: 6px; | ||
| border-top-left-radius: 0; | ||
| border-top-right-radius: 0; | ||
| box-shadow: 0 4px 6px rgba(0, 0, 0, 0.1); | ||
| max-height: 260px; | ||
| overflow: auto; | ||
| display: none; | ||
| z-index: 50; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Graph options panel z-index is too low; may render under other layers.
Align with .custom-dropdown (z-index: 1000) to avoid clipping.
.graph-options {
position: absolute;
top: calc(100%);
left: 0;
right: 0;
background: var(--falkor-secondary);
border: 1px solid var(--border-color);
border-radius: 6px;
border-top-left-radius: 0;
border-top-right-radius: 0;
box-shadow: 0 4px 6px rgba(0, 0, 0, 0.1);
max-height: 260px;
overflow: auto;
display: none;
- z-index: 50;
+ z-index: 1000;
}📝 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.
| .graph-options { | |
| position: absolute; | |
| top: calc(100%); | |
| left: 0; | |
| right: 0; | |
| background: var(--falkor-secondary); | |
| border: 1px solid var(--border-color); | |
| border-radius: 6px; | |
| border-top-left-radius: 0; | |
| border-top-right-radius: 0; | |
| box-shadow: 0 4px 6px rgba(0, 0, 0, 0.1); | |
| max-height: 260px; | |
| overflow: auto; | |
| display: none; | |
| z-index: 50; | |
| } | |
| .graph-options { | |
| position: absolute; | |
| top: calc(100%); | |
| left: 0; | |
| right: 0; | |
| background: var(--falkor-secondary); | |
| border: 1px solid var(--border-color); | |
| border-radius: 6px; | |
| border-top-left-radius: 0; | |
| border-top-right-radius: 0; | |
| box-shadow: 0 4px 6px rgba(0, 0, 0, 0.1); | |
| max-height: 260px; | |
| overflow: auto; | |
| display: none; | |
| z-index: 1000; | |
| } |
🤖 Prompt for AI Agents
In app/public/css/menu.css around lines 359 to 374, the .graph-options z-index
is set to 50 which can cause it to render beneath other UI layers; update its
z-index to match .custom-dropdown (z-index: 1000) so the panel appears above
other elements and avoids clipping.
| .dropdown-option { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: flex-start; | ||
| padding: 8px 12px; | ||
| gap: 8px; | ||
| color: var(--text-primary); | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .dropdown-option:hover { | ||
| background: var(--bg-tertiary); | ||
| } | ||
|
|
||
| .dropdown-option span { | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; | ||
| } | ||
|
|
||
| .dropdown-option .delete-btn { | ||
| background: transparent; | ||
| border: none; | ||
| color: #ff6b6b; | ||
| opacity: 0; | ||
| cursor: pointer; | ||
| width: 28px; | ||
| height: 28px; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| margin-left: auto; | ||
| } | ||
|
|
||
| .dropdown-option:hover .delete-btn { | ||
| opacity: 1; | ||
| } | ||
|
|
||
| .dropdown-option .delete-btn svg { | ||
| width: 16px; | ||
| height: 16px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Selector collision with global .dropdown-option. Scope styles to graph menu.
Prevent unintended overrides of other dropdowns.
-.dropdown-option {
+.graph-options .dropdown-option {
display: flex;
align-items: center;
justify-content: flex-start;
padding: 8px 12px;
gap: 8px;
color: var(--text-primary);
cursor: pointer;
}
-.dropdown-option:hover {
+.graph-options .dropdown-option:hover {
background: var(--bg-tertiary);
}
-.dropdown-option span {
+.graph-options .dropdown-option span {
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
}
-.dropdown-option .delete-btn {
+.graph-options .dropdown-option .delete-btn {
background: transparent;
border: none;
color: #ff6b6b;
opacity: 0;
cursor: pointer;
width: 28px;
height: 28px;
display: flex;
align-items: center;
justify-content: center;
margin-left: auto;
}
-.dropdown-option:hover .delete-btn {
+.graph-options .dropdown-option:hover .delete-btn {
opacity: 1;
}
-.dropdown-option .delete-btn svg {
+.graph-options .dropdown-option .delete-btn svg {
width: 16px;
height: 16px;
}🤖 Prompt for AI Agents
In app/public/css/menu.css around lines 376 to 417, the current .dropdown-option
rules are global and may collide with other dropdown styles; scope these
selectors to the graph menu by prefixing every rule with the graph menu
container class (e.g. .graph-menu .dropdown-option, .graph-menu
.dropdown-option:hover, .graph-menu .dropdown-option span, .graph-menu
.dropdown-option .delete-btn, etc.), update the hover and descendant selectors
accordingly so the styles only apply within the graph menu, and ensure you add
or use the correct container class in the HTML if it doesn't exist yet.
| <!-- Custom dropdown to show per-item delete action on hover --> | ||
| <div id="graph-custom-dropdown" class="graph-custom-dropdown"> | ||
| <div id="graph-selected" class="graph-selected dropdown-selected" title="Select Database"> | ||
| <span class="dropdown-text">Select Database</span> | ||
| <span class="dropdown-arrow">▼</span> | ||
| </div> | ||
| <div id="graph-options" class="graph-options dropdown-options" aria-hidden="true"></div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper a11y semantics and a real selected value (not display text).
The custom dropdown lacks keyboard support and ARIA roles, and the selected value is inferred from visible text, which will break when the placeholder (“Select Database”) is present. Store the selected value in a data attribute and apply combobox/listbox semantics.
Apply:
- <div id="graph-custom-dropdown" class="graph-custom-dropdown">
- <div id="graph-selected" class="graph-selected dropdown-selected" title="Select Database">
- <span class="dropdown-text">Select Database</span>
- <span class="dropdown-arrow">▼</span>
- </div>
- <div id="graph-options" class="graph-options dropdown-options" aria-hidden="true"></div>
- </div>
+ <div id="graph-custom-dropdown" class="graph-custom-dropdown">
+ <div
+ id="graph-selected"
+ class="graph-selected dropdown-selected"
+ title="Select Database"
+ role="combobox"
+ aria-haspopup="listbox"
+ aria-expanded="false"
+ aria-controls="graph-options"
+ tabindex="0"
+ data-value=""
+ >
+ <span class="dropdown-text">Select Database</span>
+ <span class="dropdown-arrow" aria-hidden="true">▼</span>
+ </div>
+ <div id="graph-options" class="graph-options dropdown-options" role="listbox" aria-hidden="true"></div>
+ </div>Follow-up: ensure the TS toggles aria-expanded/aria-hidden when opening/closing, and that getSelectedGraph() reads graph-selected.dataset.value instead of text.
📝 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.
| <!-- Custom dropdown to show per-item delete action on hover --> | |
| <div id="graph-custom-dropdown" class="graph-custom-dropdown"> | |
| <div id="graph-selected" class="graph-selected dropdown-selected" title="Select Database"> | |
| <span class="dropdown-text">Select Database</span> | |
| <span class="dropdown-arrow">▼</span> | |
| </div> | |
| <div id="graph-options" class="graph-options dropdown-options" aria-hidden="true"></div> | |
| </div> | |
| <div id="graph-custom-dropdown" class="graph-custom-dropdown"> | |
| <div | |
| id="graph-selected" | |
| class="graph-selected dropdown-selected" | |
| title="Select Database" | |
| role="combobox" | |
| aria-haspopup="listbox" | |
| aria-expanded="false" | |
| aria-controls="graph-options" | |
| tabindex="0" | |
| data-value="" | |
| > | |
| <span class="dropdown-text">Select Database</span> | |
| <span class="dropdown-arrow" aria-hidden="true">▼</span> | |
| </div> | |
| <div id="graph-options" class="graph-options dropdown-options" role="listbox" aria-hidden="true"></div> | |
| </div> |
| export function getSelectedGraph(): string | null { | ||
| const selectedLabel = document.getElementById('graph-selected'); | ||
| const text = selectedLabel?.querySelector('.dropdown-text')?.textContent; | ||
| if (text) return text; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Trim value and ignore placeholder text.
Prevents treating “Select a graph” (or similar) as a real selection.
export function getSelectedGraph(): string | null {
- const selectedLabel = document.getElementById('graph-selected');
- const text = selectedLabel?.querySelector('.dropdown-text')?.textContent;
- if (text) return text;
- return null;
+ const selectedLabel = document.getElementById('graph-selected');
+ const text = (selectedLabel?.querySelector('.dropdown-text')?.textContent || '').trim();
+ const placeholder = (selectedLabel?.getAttribute('data-placeholder') || '').trim();
+ if (!text || (placeholder && text === placeholder)) return null;
+ return text;
}📝 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.
| export function getSelectedGraph(): string | null { | |
| const selectedLabel = document.getElementById('graph-selected'); | |
| const text = selectedLabel?.querySelector('.dropdown-text')?.textContent; | |
| if (text) return text; | |
| return null; | |
| } | |
| export function getSelectedGraph(): string | null { | |
| const selectedLabel = document.getElementById('graph-selected'); | |
| const text = (selectedLabel?.querySelector('.dropdown-text')?.textContent || '').trim(); | |
| const placeholder = (selectedLabel?.getAttribute('data-placeholder') || '').trim(); | |
| if (!text || (placeholder && text === placeholder)) return null; | |
| return text; | |
| } |
🤖 Prompt for AI Agents
In app/ts/modules/graph_select.ts around lines 7 to 12, the function returns the
raw textContent which can include whitespace or the placeholder "Select a
graph"; update it to trim the text and treat empty or placeholder values as null
— e.g., get textContent, call .trim(), if the trimmed string is empty or matches
a placeholder pattern like /^select\b/i (or explicit "Select a graph") return
null, otherwise return the trimmed string.
| export function addGraphOption(name: string, onSelect: (n: string) => void, onDelete: (n: string) => void) { | ||
| const optionsContainer = document.getElementById('graph-options'); | ||
| if (!optionsContainer) return; | ||
| const row = document.createElement('div'); | ||
| row.className = 'dropdown-option'; | ||
| row.setAttribute('data-value', name); | ||
| const icon = document.createElement('span'); | ||
| icon.className = 'db-icon'; | ||
| // optional: could add icons later | ||
| const text = document.createElement('span'); | ||
| text.textContent = name; | ||
| row.appendChild(icon); | ||
| row.appendChild(text); | ||
|
|
||
| const delBtn = document.createElement('button'); | ||
| delBtn.className = 'delete-btn'; | ||
| delBtn.title = `Delete ${name}`; | ||
| delBtn.innerHTML = `<svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="3 6 5 6 21 6"></polyline><path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"></path><path d="M10 11v6"></path><path d="M14 11v6"></path><path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"></path></svg>`; | ||
| row.appendChild(delBtn); | ||
|
|
||
| row.addEventListener('click', () => { | ||
| setSelectedGraph(name); | ||
| onSelect(name); | ||
| optionsContainer.classList.remove('open'); | ||
| }); | ||
|
|
||
| delBtn.addEventListener('click', (ev) => { | ||
| ev.stopPropagation(); | ||
| onDelete(name); | ||
| }); | ||
|
|
||
| optionsContainer.appendChild(row); | ||
| } |
There was a problem hiding this comment.
Avoid innerHTML for SVG and set button type/ARIA.
Mitigates XSS lint warnings and prevents accidental form submission.
export function addGraphOption(name: string, onSelect: (n: string) => void, onDelete: (n: string) => void) {
const optionsContainer = document.getElementById('graph-options');
if (!optionsContainer) return;
const row = document.createElement('div');
row.className = 'dropdown-option';
row.setAttribute('data-value', name);
const icon = document.createElement('span');
icon.className = 'db-icon';
// optional: could add icons later
const text = document.createElement('span');
text.textContent = name;
row.appendChild(icon);
row.appendChild(text);
const delBtn = document.createElement('button');
delBtn.className = 'delete-btn';
delBtn.title = `Delete ${name}`;
- delBtn.innerHTML = `<svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="3 6 5 6 21 6"></polyline><path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"></path><path d="M10 11v6"></path><path d="M14 11v6"></path><path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"></path></svg>`;
+ delBtn.type = 'button';
+ delBtn.setAttribute('aria-label', `Delete ${name}`);
+ const svgNS = 'http://www.w3.org/2000/svg';
+ const svg = document.createElementNS(svgNS, 'svg');
+ svg.setAttribute('viewBox', '0 0 24 24');
+ svg.setAttribute('fill', 'none');
+ svg.setAttribute('stroke', 'currentColor');
+ svg.setAttribute('stroke-width', '2');
+ svg.setAttribute('stroke-linecap', 'round');
+ svg.setAttribute('stroke-linejoin', 'round');
+ const poly = document.createElementNS(svgNS, 'polyline');
+ poly.setAttribute('points', '3 6 5 6 21 6');
+ const p1 = document.createElementNS(svgNS, 'path');
+ p1.setAttribute('d', 'M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6');
+ const p2 = document.createElementNS(svgNS, 'path');
+ p2.setAttribute('d', 'M10 11v6');
+ const p3 = document.createElementNS(svgNS, 'path');
+ p3.setAttribute('d', 'M14 11v6');
+ const p4 = document.createElementNS(svgNS, 'path');
+ p4.setAttribute('d', 'M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2');
+ svg.append(poly, p1, p2, p3, p4);
+ delBtn.appendChild(svg);
row.appendChild(delBtn);
row.addEventListener('click', () => {
setSelectedGraph(name);
onSelect(name);
optionsContainer.classList.remove('open');
});
delBtn.addEventListener('click', (ev) => {
ev.stopPropagation();
onDelete(name);
});
optionsContainer.appendChild(row);
}📝 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.
| export function addGraphOption(name: string, onSelect: (n: string) => void, onDelete: (n: string) => void) { | |
| const optionsContainer = document.getElementById('graph-options'); | |
| if (!optionsContainer) return; | |
| const row = document.createElement('div'); | |
| row.className = 'dropdown-option'; | |
| row.setAttribute('data-value', name); | |
| const icon = document.createElement('span'); | |
| icon.className = 'db-icon'; | |
| // optional: could add icons later | |
| const text = document.createElement('span'); | |
| text.textContent = name; | |
| row.appendChild(icon); | |
| row.appendChild(text); | |
| const delBtn = document.createElement('button'); | |
| delBtn.className = 'delete-btn'; | |
| delBtn.title = `Delete ${name}`; | |
| delBtn.innerHTML = `<svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="3 6 5 6 21 6"></polyline><path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"></path><path d="M10 11v6"></path><path d="M14 11v6"></path><path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"></path></svg>`; | |
| row.appendChild(delBtn); | |
| row.addEventListener('click', () => { | |
| setSelectedGraph(name); | |
| onSelect(name); | |
| optionsContainer.classList.remove('open'); | |
| }); | |
| delBtn.addEventListener('click', (ev) => { | |
| ev.stopPropagation(); | |
| onDelete(name); | |
| }); | |
| optionsContainer.appendChild(row); | |
| } | |
| export function addGraphOption( | |
| name: string, | |
| onSelect: (n: string) => void, | |
| onDelete: (n: string) => void | |
| ) { | |
| const optionsContainer = document.getElementById('graph-options'); | |
| if (!optionsContainer) return; | |
| const row = document.createElement('div'); | |
| row.className = 'dropdown-option'; | |
| row.setAttribute('data-value', name); | |
| const icon = document.createElement('span'); | |
| icon.className = 'db-icon'; | |
| // optional: could add icons later | |
| const text = document.createElement('span'); | |
| text.textContent = name; | |
| row.appendChild(icon); | |
| row.appendChild(text); | |
| const delBtn = document.createElement('button'); | |
| delBtn.className = 'delete-btn'; | |
| delBtn.title = `Delete ${name}`; | |
| delBtn.type = 'button'; | |
| delBtn.setAttribute('aria-label', `Delete ${name}`); | |
| const svgNS = 'http://www.w3.org/2000/svg'; | |
| const svg = document.createElementNS(svgNS, 'svg'); | |
| svg.setAttribute('viewBox', '0 0 24 24'); | |
| svg.setAttribute('fill', 'none'); | |
| svg.setAttribute('stroke', 'currentColor'); | |
| svg.setAttribute('stroke-width', '2'); | |
| svg.setAttribute('stroke-linecap', 'round'); | |
| svg.setAttribute('stroke-linejoin', 'round'); | |
| const poly = document.createElementNS(svgNS, 'polyline'); | |
| poly.setAttribute('points', '3 6 5 6 21 6'); | |
| const p1 = document.createElementNS(svgNS, 'path'); | |
| p1.setAttribute('d', 'M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6'); | |
| const p2 = document.createElementNS(svgNS, 'path'); | |
| p2.setAttribute('d', 'M10 11v6'); | |
| const p3 = document.createElementNS(svgNS, 'path'); | |
| p3.setAttribute('d', 'M14 11v6'); | |
| const p4 = document.createElementNS(svgNS, 'path'); | |
| p4.setAttribute( | |
| 'd', | |
| 'M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2' | |
| ); | |
| svg.append(poly, p1, p2, p3, p4); | |
| delBtn.appendChild(svg); | |
| row.appendChild(delBtn); | |
| row.addEventListener('click', () => { | |
| setSelectedGraph(name); | |
| onSelect(name); | |
| optionsContainer.classList.remove('open'); | |
| }); | |
| delBtn.addEventListener('click', (ev) => { | |
| ev.stopPropagation(); | |
| onDelete(name); | |
| }); | |
| optionsContainer.appendChild(row); | |
| } |
🧰 Tools
🪛 ast-grep (0.38.6)
[warning] 41-41: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: delBtn.innerHTML = <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="3 6 5 6 21 6"></polyline><path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"></path><path d="M10 11v6"></path><path d="M14 11v6"></path><path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"></path></svg>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 41-41: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: delBtn.innerHTML = <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="3 6 5 6 21 6"></polyline><path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"></path><path d="M10 11v6"></path><path d="M14 11v6"></path><path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"></path></svg>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
In app/ts/modules/graph_select.ts around lines 25 to 57, the delete button is
created using innerHTML for the SVG and no explicit button type or ARIA
attributes, which triggers XSS/lint warnings and allows accidental form
submission; replace innerHTML by constructing the SVG via
document.createElementNS('http://www.w3.org/2000/svg', ...) and append the
necessary SVG child elements (paths/polyline) with appropriate attributes, set
delBtn.type = 'button' and add an accessible label such as
delBtn.setAttribute('aria-label', `Delete ${name}`) (or use title if needed) so
the button is non-submitting and accessible.
| // Clear custom dropdown and show no graphs state | ||
| clearGraphOptions(); | ||
|
|
||
| if (DOM.messageInput) DOM.messageInput.disabled = true; | ||
| if (DOM.submitButton) DOM.submitButton.disabled = true; | ||
| if (DOM.messageInput) DOM.messageInput.placeholder = 'Please upload a schema or connect a database to start chatting'; | ||
|
|
||
| addMessage('No graphs are available. Please upload a schema file or connect to a database to get started.', false); | ||
|
|
||
| // Update the visible selected label to show no graphs state | ||
| const selectedLabel = document.getElementById('graph-selected'); | ||
| if (selectedLabel) { | ||
| const dropdownText = selectedLabel.querySelector('.dropdown-text'); | ||
| if (dropdownText) { | ||
| dropdownText.textContent = 'No Databases'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set dropdown to an unselected state (value + ARIA) when no graphs exist.
Only changing the visible text to “No Databases” is insufficient; keep state consistent and accessible.
- const selectedLabel = document.getElementById('graph-selected');
- if (selectedLabel) {
- const dropdownText = selectedLabel.querySelector('.dropdown-text');
- if (dropdownText) {
- dropdownText.textContent = 'No Databases';
- }
- }
+ const selectedLabel = document.getElementById('graph-selected') as HTMLElement | null;
+ const optionsEl = document.getElementById('graph-options') as HTMLElement | null;
+ if (selectedLabel) {
+ const dropdownText = selectedLabel.querySelector('.dropdown-text');
+ if (dropdownText) (dropdownText as HTMLElement).textContent = 'No Databases';
+ selectedLabel.dataset.value = '';
+ selectedLabel.setAttribute('aria-expanded', 'false');
+ }
+ if (optionsEl) {
+ optionsEl.classList.remove('open');
+ optionsEl.setAttribute('aria-hidden', 'true');
+ }📝 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.
| // Clear custom dropdown and show no graphs state | |
| clearGraphOptions(); | |
| if (DOM.messageInput) DOM.messageInput.disabled = true; | |
| if (DOM.submitButton) DOM.submitButton.disabled = true; | |
| if (DOM.messageInput) DOM.messageInput.placeholder = 'Please upload a schema or connect a database to start chatting'; | |
| addMessage('No graphs are available. Please upload a schema file or connect to a database to get started.', false); | |
| // Update the visible selected label to show no graphs state | |
| const selectedLabel = document.getElementById('graph-selected'); | |
| if (selectedLabel) { | |
| const dropdownText = selectedLabel.querySelector('.dropdown-text'); | |
| if (dropdownText) { | |
| dropdownText.textContent = 'No Databases'; | |
| } | |
| } | |
| // Update the visible selected label to show no graphs state | |
| - const selectedLabel = document.getElementById('graph-selected'); | |
| - if (selectedLabel) { | |
| - const dropdownText = selectedLabel.querySelector('.dropdown-text'); | |
| - if (dropdownText) { | |
| - dropdownText.textContent = 'No Databases'; | |
| - } | |
| const selectedLabel = document.getElementById('graph-selected') as HTMLElement | null; | |
| const optionsEl = document.getElementById('graph-options') as HTMLElement | null; | |
| if (selectedLabel) { | |
| const dropdownText = selectedLabel.querySelector('.dropdown-text'); | |
| if (dropdownText) (dropdownText as HTMLElement).textContent = 'No Databases'; | |
| // Clear selection state and collapse dropdown for accessibility | |
| selectedLabel.dataset.value = ''; | |
| selectedLabel.setAttribute('aria-expanded', 'false'); | |
| } | |
| if (optionsEl) { | |
| optionsEl.classList.remove('open'); | |
| optionsEl.setAttribute('aria-hidden', 'true'); | |
| } |
| // populate custom dropdown | ||
| try { | ||
| clearGraphOptions(); | ||
| data.forEach(graph => { | ||
| addGraphOption(graph, (name) => { | ||
| // onSelect | ||
| setSelectedGraph(name); | ||
| initChat(); | ||
| }, async (name) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dispatch a “graph-selected” event and stop propagation; don’t call initChat here.
Let app.ts handle chat reset and schema re-load in one place; also prevent the container click from firing.
- addGraphOption(graph, (name) => {
- // onSelect
- setSelectedGraph(name);
- initChat();
- }, async (name) => {
+ addGraphOption(graph, (name) => {
+ // onSelect
+ setSelectedGraph(name);
+ document.dispatchEvent(new CustomEvent('graph-selected', { detail: name }));
+ }, async (name) => {Additionally update addGraphOption in modules/graph_select.ts to stop propagation on row click:
// In graph_select.ts addGraphOption:
row.addEventListener('click', (ev) => {
ev.stopPropagation();
setSelectedGraph(name);
onSelect(name);
optionsContainer.classList.remove('open');
});🤖 Prompt for AI Agents
In app/ts/modules/graphs.ts around lines 65 to 73, the graph option click
handler currently calls initChat directly and doesn't stop propagation; instead
remove the initChat() call, dispatch a custom "graph-selected" event with the
selected graph name as detail, and call ev.stopPropagation() (or ensure the
click handler passed in will stop propagation) so the container click doesn't
also fire; also update addGraphOption in app/ts/modules/graph_select.ts so the
row click listener stops propagation, sets the selected graph, invokes the
provided onSelect callback, and closes the options container (i.e., move
stopping propagation into addGraphOption as shown in the reviewer comment).
| const sel = document.getElementById('graph-selected'); | ||
| if (sel) sel.addEventListener('click', () => toggleOptions()); | ||
| } catch (e) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid accumulating multiple click handlers on the selected label.
loadGraphs() can be called repeatedly; addEventListener here will stack handlers. Replace with a single assignment.
- const sel = document.getElementById('graph-selected');
- if (sel) sel.addEventListener('click', () => toggleOptions());
+ const sel = document.getElementById('graph-selected') as HTMLElement | null;
+ if (sel) sel.onclick = () => toggleOptions();📝 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.
| const sel = document.getElementById('graph-selected'); | |
| if (sel) sel.addEventListener('click', () => toggleOptions()); | |
| } catch (e) { | |
| const sel = document.getElementById('graph-selected') as HTMLElement | null; | |
| if (sel) sel.onclick = () => toggleOptions(); |
🤖 Prompt for AI Agents
In app/ts/modules/graphs.ts around lines 94 to 96, the click handler for the
element with id 'graph-selected' is added with addEventListener each time
loadGraphs() runs, causing multiple handlers to accumulate; replace this with a
single assignment (e.g., element.onclick = () => toggleOptions()) or explicitly
remove the previous listener before adding to ensure only one handler is
attached.
| async function onDeleteClick() { | ||
| const graphName = getSelectedGraph(); | ||
| if (!graphName) { | ||
| addMessage('Please select a graph to delete.', false); | ||
| return; | ||
| } | ||
|
|
||
| const confirmed = confirm(`Are you sure you want to delete the graph '${graphName}'? This action cannot be undone.`); | ||
| if (!confirmed) return; | ||
|
|
||
| try { | ||
| const resp = await fetch(`/graphs/${encodeURIComponent(graphName)}`, { method: 'DELETE' }); | ||
| if (!resp.ok) { | ||
| const text = await resp.text(); | ||
| throw new Error(`Failed to delete graph: ${resp.status} ${text}`); | ||
| } | ||
| addMessage(`Graph '${graphName}' deleted.`, false); | ||
| // Clear current chat state if the deleted graph was selected | ||
| if (window && (window as any).currentGraph === graphName) { | ||
| (window as any).currentGraph = undefined; | ||
| } | ||
| } catch (err) { | ||
| console.error('Error deleting graph:', err); | ||
| addMessage('Error deleting graph: ' + (err as Error).message, false); | ||
| } finally { | ||
| // Always reload graphs list after delete attempt | ||
| loadGraphs(); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
After deletion, reset the dropdown’s selected state.
If the current graph is deleted, also clear the selected value/label immediately to avoid stale UI.
addMessage(`Graph '${graphName}' deleted.`, false);
// Clear current chat state if the deleted graph was selected
if (window && (window as any).currentGraph === graphName) {
(window as any).currentGraph = undefined;
}
+ // Reset dropdown selection
+ const selectedLabel = document.getElementById('graph-selected') as HTMLElement | null;
+ if (selectedLabel) {
+ selectedLabel.dataset.value = '';
+ const txt = selectedLabel.querySelector('.dropdown-text');
+ if (txt) (txt as HTMLElement).textContent = 'Select Database';
+ selectedLabel.setAttribute('aria-expanded', 'false');
+ }To ensure this global is still needed, search for currentGraph usage:
🏁 Script executed:
#!/bin/bash
rg -n -C2 -g 'app/**' -P '\bcurrentGraph\b'Length of output: 501
🏁 Script executed:
#!/bin/bash
# Find the selector logic for graph dropdown
rg -n 'getSelectedGraph' -g 'app/**'
# Inspect how graphs are loaded into the UI
rg -n 'function loadGraphs' -g 'app/**'
# Verify existence of the suggested dropdown ID
rg -n 'graph-selected' -g 'app/**'
# Check for the dropdown-text class in the codebase
rg -n 'dropdown-text' -g 'app/**'Length of output: 2473
🏁 Script executed:
#!/bin/bash
# Extract the implementation of setSelectedGraph to verify UI reset behavior
rg -nP -C5 'function setSelectedGraph' -g 'app/ts/modules/graph_select.ts'Length of output: 809
🏁 Script executed:
#!/bin/bash
# Inspect clearGraphOptions implementation
rg -nP -C10 'export function clearGraphOptions' -g 'app/ts/modules/graph_select.ts'Length of output: 1595
Reset dropdown selection after successful graph deletion
Add immediately after clearing currentGraph when the deleted graph was selected:
if (window && (window as any).currentGraph === graphName) {
(window as any).currentGraph = undefined;
}
+ // Reset dropdown selection
+ const selectedLabel = document.getElementById('graph-selected') as HTMLElement | null;
+ if (selectedLabel) {
+ selectedLabel.dataset.value = '';
+ const txt = selectedLabel.querySelector('.dropdown-text');
+ if (txt) (txt as HTMLElement).textContent = 'Select Database';
+ selectedLabel.setAttribute('aria-expanded', 'false');
+ }📝 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.
| async function onDeleteClick() { | |
| const graphName = getSelectedGraph(); | |
| if (!graphName) { | |
| addMessage('Please select a graph to delete.', false); | |
| return; | |
| } | |
| const confirmed = confirm(`Are you sure you want to delete the graph '${graphName}'? This action cannot be undone.`); | |
| if (!confirmed) return; | |
| try { | |
| const resp = await fetch(`/graphs/${encodeURIComponent(graphName)}`, { method: 'DELETE' }); | |
| if (!resp.ok) { | |
| const text = await resp.text(); | |
| throw new Error(`Failed to delete graph: ${resp.status} ${text}`); | |
| } | |
| addMessage(`Graph '${graphName}' deleted.`, false); | |
| // Clear current chat state if the deleted graph was selected | |
| if (window && (window as any).currentGraph === graphName) { | |
| (window as any).currentGraph = undefined; | |
| } | |
| } catch (err) { | |
| console.error('Error deleting graph:', err); | |
| addMessage('Error deleting graph: ' + (err as Error).message, false); | |
| } finally { | |
| // Always reload graphs list after delete attempt | |
| loadGraphs(); | |
| } | |
| } | |
| async function onDeleteClick() { | |
| const graphName = getSelectedGraph(); | |
| if (!graphName) { | |
| addMessage('Please select a graph to delete.', false); | |
| return; | |
| } | |
| const confirmed = confirm( | |
| `Are you sure you want to delete the graph '${graphName}'? This action cannot be undone.` | |
| ); | |
| if (!confirmed) return; | |
| try { | |
| const resp = await fetch(`/graphs/${encodeURIComponent(graphName)}`, { | |
| method: 'DELETE' | |
| }); | |
| if (!resp.ok) { | |
| const text = await resp.text(); | |
| throw new Error(`Failed to delete graph: ${resp.status} ${text}`); | |
| } | |
| addMessage(`Graph '${graphName}' deleted.`, false); | |
| // Clear current chat state if the deleted graph was selected | |
| if (window && (window as any).currentGraph === graphName) { | |
| (window as any).currentGraph = undefined; | |
| } | |
| // Reset dropdown selection | |
| const selectedLabel = document.getElementById( | |
| 'graph-selected' | |
| ) as HTMLElement | null; | |
| if (selectedLabel) { | |
| selectedLabel.dataset.value = ''; | |
| const txt = selectedLabel.querySelector('.dropdown-text'); | |
| if (txt) { | |
| (txt as HTMLElement).textContent = 'Select Database'; | |
| } | |
| selectedLabel.setAttribute('aria-expanded', 'false'); | |
| } | |
| } catch (err) { | |
| console.error('Error deleting graph:', err); | |
| addMessage('Error deleting graph: ' + (err as Error).message, false); | |
| } finally { | |
| // Always reload graphs list after delete attempt | |
| loadGraphs(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/ts/modules/graphs.ts around lines 132 to 160, after you clear
window.currentGraph when the deleted graph was selected, also reset the graph
selection dropdown UI so it reflects that no graph is selected: find the select
element used by getSelectedGraph (e.g. by id or class), set its value to the
default/empty selection and trigger any change handlers if necessary; ensure
this happens only on successful deletion and before calling loadGraphs().
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
api/routes/auth.py (6)
271-273: Typo: “Github” → “GitHub” (user-facing text).Fix capitalization.
Apply this diff:
- raise HTTPException(status_code=400, detail="Failed to get user info from Github") + raise HTTPException(status_code=400, detail="Failed to get user info from GitHub")
169-174: Harden session cookie: add SameSite and consider dev-friendlysecurehandling.
- Add
samesite="lax"to reduce CSRF risk on the API token cookie.secure=Trueblocks cookies on HTTP during local dev; considersecure=(request.url.scheme == "https")or an env override.Example (apply to both Google and GitHub sets):
redirect.set_cookie( key="api_token", value=api_token, httponly=True, secure=(request.url.scheme == "https"), # or bool(os.getenv("FORCE_SECURE_COOKIES", "1")) samesite="lax", )Also applies to: 257-262
80-80: Wrap the long docstring to satisfy pylint line length.Current line is 127 chars. Wrap to multiline.
Suggested docstring:
"""Handle the home page. Renders the landing page for unauthenticated users and the chat page for authenticated users. """
107-109: Add a brief docstring forlogin_page(pylint warning).This will clear “Missing function or method docstring”.
Suggested docstring:
"""Redirects to the default OAuth provider (Google) login."""
220-225: Avoid logging full response bodies from OAuth providers.
resp.textmay include PII. Log status and a sanitized error summary instead.Example:
logging.error("Failed to fetch GitHub user info: status=%s", resp.status_code)
186-189: Prefer narrower exception handling over blanketException.Catching specific exceptions (e.g.,
httpx.HTTPError, Authlib’sOAuthError) improves clarity and avoids hiding bugs.If helpful, I can tighten these except blocks after confirming which exceptions Authlib emits in your stack. Do you want me to update them to catch
httpx.HTTPErrorandauthlib.integrations.base_client.errors.OAuthErrorspecifically?Also applies to: 275-277
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
api/routes/auth.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/routes/auth.py
🪛 Ruff (0.12.2)
api/routes/auth.py
180-180: Abstract raise to an inner function
(TRY301)
184-184: Abstract raise to an inner function
(TRY301)
186-186: Do not catch blind exception: Exception
(BLE001)
187-187: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
187-187: Use explicit conversion flag
Replace with conversion flag
(RUF010)
268-268: Abstract raise to an inner function
(TRY301)
272-272: Abstract raise to an inner function
(TRY301)
274-274: Do not catch blind exception: Exception
(BLE001)
275-275: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
275-275: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🪛 GitHub Actions: Pylint
api/routes/auth.py
[warning] 80-80: Pylint: Line too long (127/100) (line-too-long).
[warning] 107-107: Pylint: Missing function or method docstring (missing-function-docstring).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: deploy
- GitHub Check: e2e-tests
🔇 Additional comments (2)
api/routes/auth.py (2)
243-243: Good: canonical GitHub email resolution is used.Using the derived
/user/emailswhen needed) is the correct behavior.
160-167: callback_handler is async; await usage is correct. Verified that app.state.callback_handler is assigned only to the async function handle_callback (in api/auth/oauth_handlers.py line 22); no sync variants exist, so the unguardedawait handler(...)is appropriate.
| # Call the registered handler (await if async) | ||
| await handler('google', user_data, api_token) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t unconditionally await the callback handler (support sync and async).
await handler(...) will raise if the handler is synchronous. Detect coroutine functions and call accordingly.
Apply this diff:
- # Call the registered handler (await if async)
- await handler('google', user_data, api_token)
+ # Call the registered handler (await if async)
+ if inspect.iscoroutinefunction(handler):
+ await handler('google', user_data, api_token)
+ else:
+ handler('google', user_data, api_token)Add the missing import near the top-level imports:
import inspect🤖 Prompt for AI Agents
In api/routes/auth.py around lines 165 to 167, the code unconditionally does
`await handler(...)` which will fail for synchronous handlers; add `import
inspect` to the top-level imports and replace the unconditional await with a
conditional call that checks `inspect.iscoroutinefunction(handler)` (or
`inspect.isawaitable(...)` if passing a returned value) and `await`s only when
appropriate, otherwise call the handler synchronously and use its return value.
| # Handler not set - log and raise error to prevent silent failure | ||
| logging.error("Google OAuth callback handler not registered in app state") | ||
| raise HTTPException(status_code=500, detail="Authentication handler not configured") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
HTTPException raised inside try is swallowed by the broad except below.
Raising here gets caught by the later except Exception and re-mapped to 400, masking the intended 500. Either narrow the try scope or re-raise HTTPException in the except block (see next comment’s diff).
🧰 Tools
🪛 Ruff (0.12.2)
180-180: Abstract raise to an inner function
(TRY301)
🤖 Prompt for AI Agents
In api/routes/auth.py around lines 178 to 181, the code raises an HTTPException
when the Google OAuth callback handler is missing but that exception is
swallowed by a later broad except which maps everything to a 400; narrow the try
block so the HTTPException is raised outside of the broad except, or modify the
except to detect and re-raise HTTPException (e.g., if isinstance(e,
HTTPException): raise) so the original 500 status is preserved.
| logging.warning("No user info received from Google OAuth") | ||
| raise HTTPException(status_code=400, detail="Failed to get user info from Google") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same masking risk for the “no user info” branch.
This HTTPException(400, ...) is also intercepted by the broad except Exception. Ensure HTTPException is re-raised unchanged in the except block.
🧰 Tools
🪛 Ruff (0.12.2)
184-184: Abstract raise to an inner function
(TRY301)
🤖 Prompt for AI Agents
In api/routes/auth.py around lines 183 to 185, the HTTPException("Failed to get
user info from Google") thrown when no user info is returned can be swallowed by
the surrounding broad except Exception; update the exception handling so that
inside the except block you detect if the caught exception is an instance of
HTTPException and re-raise it unchanged (e.g., if isinstance(e, HTTPException):
raise), otherwise handle/log and raise or return the converted error as
currently implemented.
| except Exception as e: | ||
| logging.error(f"Google OAuth authentication failed: {str(e)}") | ||
| raise HTTPException(status_code=400, detail=f"Authentication failed: {str(e)}") | ||
|
|
There was a problem hiding this comment.
Use logging.exception, avoid f-strings in logs, and don’t blanket-convert all errors to 400.
- Switch to
logging.exception("...", %s)for proper traceback and lazy formatting. - Preserve earlier
HTTPExceptions; return 500 for unexpected errors to avoid leaking internals as 400.
Apply this diff:
- except Exception as e:
- logging.error(f"Google OAuth authentication failed: {str(e)}")
- raise HTTPException(status_code=400, detail=f"Authentication failed: {str(e)}")
+ except HTTPException:
+ # Preserve earlier explicit HTTP errors (e.g., 400/500 above)
+ raise
+ except Exception as e:
+ logging.exception("Google OAuth authentication failed: %s", e)
+ raise HTTPException(status_code=500, detail="Authentication failed")📝 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.
| except Exception as e: | |
| logging.error(f"Google OAuth authentication failed: {str(e)}") | |
| raise HTTPException(status_code=400, detail=f"Authentication failed: {str(e)}") | |
| except HTTPException: | |
| # Preserve earlier explicit HTTP errors (e.g., 400/500 above) | |
| raise | |
| except Exception as e: | |
| logging.exception("Google OAuth authentication failed: %s", e) | |
| raise HTTPException(status_code=500, detail="Authentication failed") |
🧰 Tools
🪛 Ruff (0.12.2)
186-186: Do not catch blind exception: Exception
(BLE001)
187-187: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
187-187: Use explicit conversion flag
Replace with conversion flag
(RUF010)
188-188: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
188-188: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In api/routes/auth.py around lines 186 to 189, replace the current broad except
block that uses f-strings and always converts errors to HTTP 400; instead catch
HTTPException first and re-raise it unchanged, and for other exceptions call
logging.exception with a format string and arguments (no f-strings) to include
the traceback, then raise an HTTPException with status_code=500 and a generic
detail (do not expose internal error text).
| # Call the registered handler (await if async) | ||
| await handler('github', user_data, api_token) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror async/sync-safe handler invocation for GitHub.
Same issue as Google flow: don’t assume the handler is async.
Apply this diff:
- # Call the registered handler (await if async)
- await handler('github', user_data, api_token)
+ # Call the registered handler (await if async)
+ if inspect.iscoroutinefunction(handler):
+ await handler('github', user_data, api_token)
+ else:
+ handler('github', user_data, api_token)📝 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.
| # Call the registered handler (await if async) | |
| await handler('github', user_data, api_token) | |
| # Call the registered handler (await if async) | |
| if inspect.iscoroutinefunction(handler): | |
| await handler('github', user_data, api_token) | |
| else: | |
| handler('github', user_data, api_token) |
🤖 Prompt for AI Agents
In api/routes/auth.py around lines 253-255, the code unconditionally awaits the
GitHub handler (await handler('github', user_data, api_token)), which assumes
the handler is async; change it to a sync/async-safe invocation like in the
Google flow: call the handler and if the return value is awaitable then await it
(or detect coroutine function before calling), ensuring you import/use
inspect.isawaitable or inspect.iscoroutinefunction as in the Google handler path
so both sync and async handlers are supported.
| logging.error("GitHub OAuth callback handler not registered in app state") | ||
| raise HTTPException(status_code=500, detail="Authentication handler not configured") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same masking risk: handler-not-configured 500 gets turned into 400 by the blanket except.
Address by preserving HTTPException in the except block (see diff below in Lines 275–277).
🧰 Tools
🪛 Ruff (0.12.2)
268-268: Abstract raise to an inner function
(TRY301)
🤖 Prompt for AI Agents
In api/routes/auth.py around lines 267-269 (and the surrounding except at
~275-277), the blanket except is masking HTTPException and converting configured
500s into 400s; update the except handler so that if the caught exception is an
instance of fastapi.HTTPException you re-raise it unchanged, otherwise log the
error and raise a new HTTPException(status_code=500, detail="Authentication
handler not configured") — preserve original exception types by checking
isinstance(exc, HTTPException) and re-raising when true.
| logging.error(f"GitHub OAuth authentication failed: {str(e)}") | ||
| raise HTTPException(status_code=400, detail=f"Authentication failed: {str(e)}") | ||
|
|
There was a problem hiding this comment.
Use logging.exception and avoid swallowing prior HTTPExceptions; prefer 500 for unexpected errors.
Same improvements as Google branch.
Apply this diff:
- except Exception as e:
- logging.error(f"GitHub OAuth authentication failed: {str(e)}")
- raise HTTPException(status_code=400, detail=f"Authentication failed: {str(e)}")
+ except HTTPException:
+ raise
+ except Exception as e:
+ logging.exception("GitHub OAuth authentication failed: %s", e)
+ raise HTTPException(status_code=500, detail="Authentication failed")📝 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.
| logging.error(f"GitHub OAuth authentication failed: {str(e)}") | |
| raise HTTPException(status_code=400, detail=f"Authentication failed: {str(e)}") | |
| except HTTPException: | |
| raise | |
| except Exception as e: | |
| logging.exception("GitHub OAuth authentication failed: %s", e) | |
| raise HTTPException(status_code=500, detail="Authentication failed") |
🧰 Tools
🪛 Ruff (0.12.2)
275-275: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
275-275: Use explicit conversion flag
Replace with conversion flag
(RUF010)
276-276: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
276-276: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In api/routes/auth.py around lines 275 to 277, replace the current except
handling that logs with logging.error and always raises HTTPException(400) so
that you use logging.exception(...) to capture the stack trace and avoid
swallowing pre-existing HTTPExceptions; specifically, in the except block, if
the caught exception is already an HTTPException re-raise it unchanged,
otherwise log the full exception with logging.exception and raise a new
HTTPException with status_code=500 and a concise detail message like
"Authentication failed: {err}".
1 participant
Summary by CodeRabbit