Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds an authenticated GET endpoint to return user-scoped graph nodes/links JSON, introduces a left vertical toolbar and schema sidebar, adds a Force Graph renderer (client), refactors sidebar toggling to multi-container control with keyboard-accessible toolbar, and updates templates/CSS to integrate the new UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser as Browser UI
participant API as GET /graphs/{id}/data
participant DB as Database
User->>Browser: Click "Schema" button
Browser->>Browser: toggleContainer(schemaContainer, onOpen)
alt opened & graph selected
Browser->>API: GET /graphs/{encodedId}/data (auth token)
API->>DB: select_graph(namespaced_id) + Cypher queries
DB-->>API: nodes & links
API-->>Browser: 200 {nodes, links}
Browser->>Browser: validate shape
Browser->>Browser: showGraph(data)
else no graph selected
Browser->>Browser: skip fetch
else API/db error
Browser->>Browser: log error / skip rendering
end
sequenceDiagram
participant UI as Browser UI
participant SB as .sidebar-container[*]
UI->>SB: toggleContainer(target, onOpen?)
Note right of SB: Close other .sidebar-container instances
alt Opening
SB->>SB: add .open (desktop padding adjust)
SB-->>UI: optional onOpen callback executed (e.g., load graph)
else Closing
SB->>SB: remove .open (restore padding)
end
UI->>UI: window resize -> adjust padding for each sidebar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (20)
api/templates/base.j2 (1)
19-20: Pin CDN versions, adddefer, and consider SRI/CSP alignmentUnpinned CDN URLs can break unexpectedly on upstream releases and block rendering. Recommend pinning versions and deferring script load. If you enforce a CSP, make sure to allow these origins or prefer local hosting with SRI.
Apply this diff:
- <script src="https://unpkg.com/d3"></script> - <script src="https://unpkg.com/force-graph"></script> + <script defer src="https://unpkg.com/d3@7"></script> + <script defer src="https://unpkg.com/force-graph@1"></script>Optional hardening (fill in integrity hashes if you stick with CDN):
<script defer src="https://unpkg.com/d3@7" integrity="<!-- sha384-... -->" crossorigin="anonymous"></script> <script defer src="https://unpkg.com/force-graph@1" integrity="<!-- sha384-... -->" crossorigin="anonymous"></script>Please verify:
- Your CSP policy allows these domains (script-src) or switch to self-hosted copies under api/static/.
- No race with modules that expect ForceGraph/d3 before DOMContentLoaded (using defer should be fine).
api/static/js/modules/schema.js (2)
4-5: Prevent multiple canvases on repeated rendersIf showGraph is called multiple times, force-graph will append another canvas each time. Clear the container or reuse a singleton instance.
Apply this diff:
- const Graph = ForceGraph()(document.getElementById('schema-graph')) + const container = document.getElementById('schema-graph'); + container.textContent = ''; + const Graph = ForceGraph()(container)Alternative: keep a module-level singleton Graph and call Graph.graphData(data) to update instead of recreating.
72-73: Reduce console noiseConsider removing or gating the console log behind a debug flag.
api/templates/components/sidebar_schema.j2 (1)
2-6: Add ARIA roles/labels for accessibilityMark the schema sidebar as a complementary region with a descriptive label.
Apply this diff:
-<div id="schema-container" class="sidebar-container"> +<div id="schema-container" class="sidebar-container" role="complementary" aria-label="Schema panel"> <div id="schema-content"> - <div id="schema-graph"></div> + <div id="schema-graph" aria-label="Graph visualization area"></div> </div> </div>api/templates/components/sidebar_menu.j2 (1)
2-2: Verify selector consistency for the menu containerYou’re using id="menu-container" plus class="sidebar-container". Ensure the JS toggling logic and CSS use the updated class-based selector, or keep the id usage consistent in config/ui modules to avoid regressions.
If moving fully to class-based selectors, consider removing the id for clarity:
-<div id="menu-container" class="sidebar-container"> +<div class="sidebar-container">api/templates/components/left_toolbar.j2 (3)
14-21: Use aria-expanded + aria-controls (not aria-pressed) for the toggle button, and wire it to the controlled containerThe “Instructions” button toggles a sidebar. For accessibility, use aria-expanded and aria-controls. aria-pressed is for on/off buttons (e.g., theme).
- <button class="action-button toolbar-button" id="menu-button" title="Instructions" aria-label="Instructions" - aria-pressed="false" tabindex="0"> + <button class="action-button toolbar-button" id="menu-button" title="Instructions" aria-label="Instructions" + aria-expanded="false" aria-controls="menu-container" tabindex="0">
43-56: Schema button should also use aria-expanded + aria-controls and maintain stateSame reasoning as the menu button: this opens/closes a panel, so aria-expanded and aria-controls are preferable.
- <button class="action-button toolbar-button" id="schema-button" title="Schema" - aria-label="Open schema" aria-pressed="false" tabindex="-1"> + <button class="action-button toolbar-button" id="schema-button" title="Schema" + aria-label="Open schema" aria-expanded="false" aria-controls="schema-container" tabindex="-1">
5-10: Minor: move presentational inline styles to CSSThe brand slot uses inline styles. Consider a small CSS rule to keep styles co-located and easier to theme.
api/routes/graphs.py (6)
71-74: Docstring says “edges” but API returns “links”Front-end expects links. Align the docstring to avoid confusion.
- This endpoint returns a JSON object with two keys: `nodes` and `edges`. - Nodes contain a minimal set of properties (id, name, labels, props). - Edges contain source and target node names (or internal ids), type and props. + This endpoint returns a JSON object with two keys: `nodes` and `links`. + Nodes contain a minimal set of properties (id, name, columns). + Links contain source and target table names.
75-80: Inconsistent graph_id length limit across endpointsThis endpoint truncates to 200 chars, while others (e.g., POST /<graph_id>) use 100. Consider a single constant to avoid divergent behavior.
Would you like me to refactor to a shared MAX_GRAPH_ID_LEN constant and update all routes accordingly?
81-86: Address CodeQL “log injection” by sanitizing user-controlled values in logsnamespaced includes user input and e can carry newlines. Sanitize before logging.
- except Exception as e: - logging.error("Failed to select graph %s: %s", namespaced, e) + except Exception as e: + safe_ns = namespaced.replace("\n", " ").replace("\r", " ")[:200] + safe_err = str(e).replace("\n", " ").replace("\r", " ")[:500] + logging.error("Failed to select graph %r: %s", safe_ns, safe_err) return jsonify({"error": "Graph not found or database error"}), 404
101-107: Also sanitize on query failure logsMirror the same sanitization for consistency and to silence the CodeQL finding.
- except Exception as e: - logging.error("Error querying graph data for %s: %s", namespaced, e) + except Exception as e: + safe_ns = namespaced.replace("\n", " ").replace("\r", " ")[:200] + safe_err = str(e).replace("\n", " ").replace("\r", " ")[:500] + logging.error("Error querying graph data for %r: %s", safe_ns, safe_err) return jsonify({"error": "Failed to read graph data"}), 500
108-123: Normalize columns to exclude nulls and enforce list shapecollect(DISTINCT c.name) may include nulls. Filter them to prevent UI oddities.
- # columns may contain nulls if no columns — normalize to empty list - if not isinstance(columns, list): - columns = [] if columns is None else [columns] + # Normalize columns: ensure a list and strip nulls + if isinstance(columns, list): + columns = [c for c in columns if c] + else: + columns = [] if columns is None else [columns] # rare driver edge-case
87-99: Be mindful of large graphs: add safeguards or paginationFor large schemas, returning all tables/links can be heavy for both DB and client. Consider:
- adding optional limits/filters (e.g., max nodes/links),
- or server-side paging,
- or feature-gating expensive queries.
If you want, I can add a max_nodes/max_links query param and hard cap on server, plus a small “truncated” flag in the response so the UI can message it.
Also applies to: 101-107, 124-137
api/static/css/responsive.css (1)
15-18: Align selector with new .sidebar-container naming (or keep both for compatibility)This still targets #menu-container. Since the rest migrated to .sidebar-container, consider updating or supporting both.
- #menu-container.open~#chat-container { + .sidebar-container.open ~ #chat-container, + #menu-container.open ~ #chat-container { margin-left: 0; }api/static/js/app.js (1)
48-49: Keep the menu button’s ARIA state in syncAfter toggling, update aria-expanded to reflect the actual state.
- DOM.menuButton.addEventListener('click', () => toggleContainer(DOM.menuContainer)); + DOM.menuButton.addEventListener('click', () => { + toggleContainer(DOM.menuContainer); + const expanded = DOM.menuContainer.classList.contains('open'); + DOM.menuButton.setAttribute('aria-expanded', expanded.toString()); + });api/static/js/modules/config.js (1)
21-29: Remove stale sideMenuButton selector to avoid drift.sideMenuButton remains in SELECTORS but is not mirrored in DOM and appears deprecated with the new toolbar. Keeping it invites confusion and dead references.
Apply this diff to remove it:
export const SELECTORS = { messageInput: '#message-input', submitButton: '#submit-button', pauseButton: '#pause-button', newChatButton: '#reset-button', chatMessages: '#chat-messages', expValue: '#exp-value', confValue: '#conf-value', missValue: '#info-value', ambValue: '#amb-value', fileUpload: '#schema-upload', fileLabel: '#custom-file-upload', - sideMenuButton: '#side-menu-button', menuButton: '#menu-button', schemaButton: '#schema-button', menuContainer: '#menu-container', schemaContainer: '#schema-container', chatContainer: '#chat-container', leftToolbar: '#left-toolbar', toolbarButtons: '#toolbar-buttons', leftToolbarInner: '#left-toolbar-inner', expInstructions: '#instructions-textarea',api/static/js/modules/ui.js (2)
19-39: Optional: keep controlling button states in sync (aria-pressed).When closing other sidebars, consider resetting any toolbar buttons with aria-pressed="true" so assistive tech reflects UI state. Can be done cheaply without hard coupling.
Example addition (after removing 'open' from other containers):
allContainers.forEach((c) => { if (c !== container && c.classList.contains('open')) { c.classList.remove('open'); + // Reset any pressed toolbar buttons + document.querySelectorAll('.toolbar-button[aria-pressed="true"]').forEach(btn => { + btn.setAttribute('aria-pressed', 'false'); + }); } });
176-225: Remove commented-out experimental code.Large commented block adds noise and will rot. Consider deleting or moving into docs if needed later.
Apply this diff to remove the block:
- // const schemaBtn = document.getElementById('toolbar-schema'); - // let schemaPanel = document.getElementById('schema-panel'); - // - // if (!schemaPanel) { - // schemaPanel = document.createElement('div'); - // schemaPanel.id = 'schema-panel'; - // schemaPanel.innerHTML = '<h3 style="margin:0 0 6px 0;font-size:14px;">Schema</h3><div id="schema-panel-body" style="margin-top:8px;font-size:13px;">Schema panel (empty). Add schema widgets here.</div>'; - // document.body.appendChild(schemaPanel); - // } - // - // async function loadSchemaIntoPanel() { - // const body = document.getElementById('schema-panel-body'); - // if (!body) return; - // body.innerHTML = '<p style="font-size:13px;margin:0;">Loading schema…</p>'; - // try { - // const resp = await fetch('/api/schema'); - // if (!resp.ok) throw new Error('Network response was not ok'); - // const data = await resp.json(); - // // If data has tables array, render a simple list/table - // if (Array.isArray(data.tables)) { - // const ul = document.createElement('ul'); - // ul.style.margin = '0'; - // ul.style.padding = '0 0 0 14px'; - // data.tables.forEach(t => { - // const li = document.createElement('li'); - // li.textContent = t.name || JSON.stringify(t); - // ul.appendChild(li); - // }); - // body.innerHTML = ''; - // body.appendChild(ul); - // } else { - // body.innerHTML = '<pre style="white-space:pre-wrap;font-size:12px;margin:0;">' + JSON.stringify(data, null, 2) + '</pre>'; - // } - // } catch (err) { - // body.innerHTML = '<p style="color:var(--text-secondary);margin:0;font-size:13px;">Could not load schema. Showing placeholder.</p><pre style="white-space:pre-wrap;font-size:12px;margin-top:8px;">' + String(err) + '</pre>'; - // } - // } - // - // if (schemaBtn) { - // schemaBtn.addEventListener('click', function() { - // const isOpen = schemaBtn.getAttribute('aria-pressed') === 'true'; - // schemaBtn.setAttribute('aria-pressed', (!isOpen).toString()); - // schemaPanel.classList.toggle('open'); - // if (!isOpen) { - // // panel opened — attempt to load schema - // loadSchemaIntoPanel(); - // } - // }); - // }api/static/css/buttons.css (1)
298-300: Remove duplicate comment line.Apply this diff:
-/* Left vertical toolbar (activity bar style) */ -/* Left vertical toolbar (activity bar style) */ +/* Left vertical toolbar (activity bar style) */
📜 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 (14)
api/routes/graphs.py(1 hunks)api/static/css/buttons.css(2 hunks)api/static/css/menu.css(3 hunks)api/static/css/responsive.css(1 hunks)api/static/js/app.js(3 hunks)api/static/js/modules/config.js(2 hunks)api/static/js/modules/schema.js(1 hunks)api/static/js/modules/ui.js(2 hunks)api/templates/base.j2(1 hunks)api/templates/chat.j2(1 hunks)api/templates/components/left_toolbar.j2(1 hunks)api/templates/components/sidebar_menu.j2(1 hunks)api/templates/components/sidebar_schema.j2(1 hunks)api/templates/components/toolbar.j2(0 hunks)
💤 Files with no reviewable changes (1)
- api/templates/components/toolbar.j2
🧰 Additional context used
📓 Path-based instructions (4)
api/templates/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Store Jinja2 templates under api/templates/
Files:
api/templates/chat.j2api/templates/base.j2api/templates/components/sidebar_schema.j2api/templates/components/left_toolbar.j2api/templates/components/sidebar_menu.j2
api/static/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Store frontend static assets under api/static/
Files:
api/static/js/modules/schema.jsapi/static/css/responsive.cssapi/static/js/modules/config.jsapi/static/js/modules/ui.jsapi/static/css/buttons.cssapi/static/css/menu.cssapi/static/js/app.js
api/routes/graphs.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Keep graph/database route handlers in api/routes/graphs.py
Files:
api/routes/graphs.py
api/routes/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Place all additional Flask route handlers under api/routes/
Files:
api/routes/graphs.py
🧬 Code Graph Analysis (3)
api/routes/graphs.py (1)
api/auth/user_management.py (1)
token_required(263-285)
api/static/js/modules/ui.js (1)
api/static/js/modules/config.js (2)
DOM(39-65)DOM(39-65)
api/static/js/app.js (3)
api/static/js/modules/config.js (2)
DOM(39-65)DOM(39-65)api/static/js/modules/ui.js (2)
toggleContainer(7-39)setupToolbar(141-225)api/static/js/modules/schema.js (1)
showGraph(2-78)
🪛 GitHub Check: CodeQL
api/routes/graphs.py
[failure] 84-84: Log Injection
This log entry depends on a user-provided value.
[failure] 105-105: Log Injection
This log entry depends on a user-provided value.
⏰ 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 (9)
api/templates/chat.j2 (1)
11-14: LGTM: Left toolbar and schema sidebar wiring looks correctIncludes are placed early in the container and should layer well with the chat content, assuming CSS sets appropriate z-index and widths.
api/templates/components/left_toolbar.j2 (1)
23-41: Theme toggle markup looks goodInline SVG covers both sun/moon/system glyphs and the button is correctly set as a toggle (aria-pressed). This matches a toggle control semantics.
api/static/css/responsive.css (1)
22-33: Verified: all menu/schema panels include class="sidebar-container"
Our scan confirmed that every element withid="menu-container"orid="schema-container"already hasclass="sidebar-container". No further changes required.api/static/js/app.js (2)
10-18: Nice: unified container toggle API and toolbar setupswitching to toggleContainer and adding setupToolbar makes the left toolbar keyboard accessible.
Also applies to: 112-118
50-84: Ensure a valid fallback dataset and synchronize ARIA stateI didn’t find any
myDatadefinition in your JS files. If it’s injected globally (e.g., in your HTML), please verify it’s available before this script runs—otherwise everyshowGraph(myData)call will throw aReferenceError. Consider providing a safe inline fallback or centralizing a demo dataset. Also, after toggling the panel, update the button’saria-expandedattribute to match its open/closed state.• Confirm or define
myData(or replace it with a proper fallback)
• Replace eachshowGraph(myData)with something likeshowGraph({ nodes: [], links: [] })or a reference to your demo dataset
• AftertoggleContainer(…), add:const expanded = DOM.schemaContainer.classList.contains('open'); DOM.schemaButton.setAttribute('aria-expanded', expanded.toString());Example diff:
try { const resp = await fetch(...); if (!resp.ok) { console.error('Failed to load graph data:', resp.status, resp.statusText); - showGraph(myData); + showGraph({ nodes: [], links: [] }); return; } const data = await resp.json(); if (!data || !Array.isArray(data.nodes) || !Array.isArray(data.links)) { console.warn('Graph data in unexpected shape, using fallback', data); - showGraph(myData); + showGraph({ nodes: [], links: [] }); return; } showGraph(data); } catch (err) { console.error('Error fetching graph data:', err); - showGraph(myData); + showGraph({ nodes: [], links: [] }); } -}); +}); +// Sync ARIA-expanded on the schema button +const expanded = DOM.schemaContainer.classList.contains('open'); +DOM.schemaButton.setAttribute('aria-expanded', expanded.toString());api/static/css/menu.css (1)
19-25: Good call: offsetting the open sidebar for the 48px left toolbarThe left: 48px aligns the opened panel with the new vertical toolbar.
api/static/js/modules/config.js (1)
52-58: All sidebar elements correctly include thesidebar-containerclass—no changes needed
- Verified IDs
schema-containerandmenu-containerexist in:
api/templates/components/sidebar_schema.j2:2(<div id="schema-container" class="sidebar-container">)api/templates/components/sidebar_menu.j2:2(<div id="menu-container" class="sidebar-container">)- Other toolbar elements (
left-toolbar,toolbar-buttons,schema-button, etc.) are present as expected.No updates required.
api/static/js/modules/ui.js (1)
7-39: No staletoggleMenu()references found
All calls totoggleMenu()have been successfully replaced bytoggleContainer(). No further action is required.api/static/css/buttons.css (1)
140-142: Confirm static placement of #theme-toggle-btn in left toolbarThe theme toggle is now rendered in api/templates/components/left_toolbar.j2 (line 23). With its CSS changed to
position: static; margin: 0;, please:
- Verify it aligns correctly alongside the other toolbar buttons (check container’s display/flex settings and spacing).
- Confirm there are no layout shifts or overlap when the header/toolbar is fixed or sticky.
- Adjust container or button-specific styles if needed to restore intended layout.
| #left-toolbar { | ||
| position: fixed; | ||
| top: 0px; | ||
| bottom: 0px; | ||
| width: 48px; | ||
| background: var(--bg-tertiary, rgba(255,255,255,0.02)); | ||
| /* border-radius: 10px; */ | ||
| padding: 6px 6px; | ||
| z-index: 1050; | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| justify-content: flex-start; | ||
| gap: 8px; | ||
| box-shadow: 0 6px 18px rgba(0,0,0,0.25); | ||
| border: 1px solid var(--border-color, rgba(255,255,255,0.03)); | ||
| backdrop-filter: blur(6px); | ||
| overflow: hidden; | ||
| } |
There was a problem hiding this comment.
Toolbar lacks a left offset; position may be unpredictable.
#left-toolbar is fixed with top/bottom but no left/right on desktop. Add a left offset so it reliably anchors to the viewport edge.
Apply this diff:
#left-toolbar {
position: fixed;
top: 0px;
bottom: 0px;
width: 48px;
+ left: 12px;📝 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.
| #left-toolbar { | |
| position: fixed; | |
| top: 0px; | |
| bottom: 0px; | |
| width: 48px; | |
| background: var(--bg-tertiary, rgba(255,255,255,0.02)); | |
| /* border-radius: 10px; */ | |
| padding: 6px 6px; | |
| z-index: 1050; | |
| display: flex; | |
| flex-direction: column; | |
| align-items: center; | |
| justify-content: flex-start; | |
| gap: 8px; | |
| box-shadow: 0 6px 18px rgba(0,0,0,0.25); | |
| border: 1px solid var(--border-color, rgba(255,255,255,0.03)); | |
| backdrop-filter: blur(6px); | |
| overflow: hidden; | |
| } | |
| #left-toolbar { | |
| position: fixed; | |
| top: 0px; | |
| bottom: 0px; | |
| width: 48px; | |
| left: 12px; | |
| background: var(--bg-tertiary, rgba(255,255,255,0.02)); | |
| /* border-radius: 10px; */ | |
| padding: 6px 6px; | |
| z-index: 1050; | |
| display: flex; | |
| flex-direction: column; | |
| align-items: center; | |
| justify-content: flex-start; | |
| gap: 8px; | |
| box-shadow: 0 6px 18px rgba(0,0,0,0.25); | |
| border: 1px solid var(--border-color, rgba(255,255,255,0.03)); | |
| backdrop-filter: blur(6px); | |
| overflow: hidden; | |
| } |
🤖 Prompt for AI Agents
In api/static/css/buttons.css around lines 300 to 318, the #left-toolbar is
positioned fixed with top and bottom but no left/right so its horizontal
placement can be unpredictable; add a left offset (e.g., left: 0;) to reliably
anchor it to the viewport edge (optionally include right: auto; if needed for
overrides) and ensure any layout breakpoints or RTL handling are accounted for
where this selector is used.
| .toolbar-button { | ||
| width: 40px; | ||
| height: 40px; | ||
| border-radius: 8px; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve affordance and focus visibility for toolbar buttons.
Add pointer cursor, normalize border/background, and provide a visible focus indicator for keyboard users.
Apply this diff:
.toolbar-button {
width: 40px;
height: 40px;
border-radius: 8px;
display: flex;
align-items: center;
justify-content: center;
box-sizing: border-box;
+ cursor: pointer;
+ border: none;
+ background: transparent;
}
+
+.toolbar-button:focus-visible {
+ outline: 2px solid var(--falkor-accent);
+ outline-offset: 2px;
+}📝 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.
| .toolbar-button { | |
| width: 40px; | |
| height: 40px; | |
| border-radius: 8px; | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| box-sizing: border-box; | |
| } | |
| .toolbar-button { | |
| width: 40px; | |
| height: 40px; | |
| border-radius: 8px; | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| box-sizing: border-box; | |
| cursor: pointer; | |
| border: none; | |
| background: transparent; | |
| } | |
| .toolbar-button:focus-visible { | |
| outline: 2px solid var(--falkor-accent); | |
| outline-offset: 2px; | |
| } |
🤖 Prompt for AI Agents
In api/static/css/buttons.css around lines 328 to 336, the .toolbar-button lacks
pointer affordance, normalized border/background and a visible keyboard focus
indicator; update the rule to add cursor: pointer, set a neutral background
(e.g. transparent or var(--btn-bg)), and reset border (border: none) while
preserving border-radius and box-sizing, then add a :focus-visible style that
provides a clear, accessible focus ring (for example a 2px solid outline or a
visible box-shadow using a high-contrast focus color and outline-offset) so
keyboard users get a clear indicator without affecting mouse interactions.
api/static/css/buttons.css
Outdated
| #schema-panel { | ||
| position: fixed; | ||
| left: 76px; | ||
| top: 84px; | ||
| width: 420px; | ||
| background: var(--bg-primary, #121212); | ||
| border: 1px solid var(--border-color, rgba(255,255,255,0.04)); | ||
| padding: 12px; | ||
| border-radius: 8px; | ||
| box-shadow: 0 8px 30px rgba(0,0,0,0.5); | ||
| display: none; | ||
| z-index: 1040; | ||
| } | ||
|
|
||
| #schema-panel.open { display: block; } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm schema container ID matches JS (schemaContainer vs schema-panel).
CSS targets #schema-panel while config.js exports schemaContainer (#schema-container). If these refer to the same element, align IDs; if not, verify both are intentional.
Run this to inspect both IDs in templates:
If they’re the same component, prefer one consistent ID across CSS/JS to prevent drift.
🏁 Script executed:
#!/bin/bash
rg -n -C2 -g '!**/dist/**' -g '!**/build/**' --type-add 'j2:*.j2' --type j2 \
'id="schema-panel"|id="schema-container"'Length of output: 453
To confirm the JS reference, let’s locate where schemaContainer is defined:
🏁 Script executed:
#!/bin/bash
rg -n -C2 -g '!**/dist/**' -g '!**/build/**' --type js 'schemaContainer'Length of output: 1297
Align Schema Panel ID Across CSS, JS, and Template
The CSS is targeting #schema-panel, but both your JS (config.js → schemaContainer: '#schema-container' / getElementById('schema-container')) and the Jinja template (<div id="schema-container">) use schema-container. This mismatch prevents the panel from ever being shown.
Files to update:
- api/static/css/buttons.css (lines ~384–398): change selector from
#schema-panelto#schema-container - api/static/js/modules/config.js: already references
schema-container - api/templates/components/sidebar_schema.j2 (line 2): already defines
id="schema-container"
Suggested CSS diff:
-#schema-panel {
+#schema-container {
position: fixed;
left: 76px;
top: 84px;
width: 420px;
/* … */
}
-#schema-panel.open { display: block; }
+#schema-container.open { display: block; }📝 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.
| #schema-panel { | |
| position: fixed; | |
| left: 76px; | |
| top: 84px; | |
| width: 420px; | |
| background: var(--bg-primary, #121212); | |
| border: 1px solid var(--border-color, rgba(255,255,255,0.04)); | |
| padding: 12px; | |
| border-radius: 8px; | |
| box-shadow: 0 8px 30px rgba(0,0,0,0.5); | |
| display: none; | |
| z-index: 1040; | |
| } | |
| #schema-panel.open { display: block; } | |
| #schema-container { | |
| position: fixed; | |
| left: 76px; | |
| top: 84px; | |
| width: 420px; | |
| background: var(--bg-primary, #121212); | |
| border: 1px solid var(--border-color, rgba(255,255,255,0.04)); | |
| padding: 12px; | |
| border-radius: 8px; | |
| box-shadow: 0 8px 30px rgba(0,0,0,0.5); | |
| display: none; | |
| z-index: 1040; | |
| } | |
| #schema-container.open { display: block; } |
🤖 Prompt for AI Agents
In api/static/css/buttons.css around lines 384–398 the CSS selector uses
#schema-panel while the JS and template use id="schema-container"; update the
selector and any related rules to #schema-container so the CSS targets the
actual DOM element, keeping the same declarations (position, sizing, background,
border, padding, radius, shadow, display, z-index) and the .open variant changed
to #schema-container.open.
| // Stop when layout stabilizes and set canvas size | ||
| Graph.onEngineStop(() => { | ||
| console.log('Engine stopped, layout stabilized.'); | ||
| }).width(window.innerWidth).height(window.innerHeight); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Size to container instead of window to avoid overflow in the sidebar
The graph canvas should match the sidebar container, not the viewport. Use the container’s bounding box.
Apply this diff:
- Graph.onEngineStop(() => {
- console.log('Engine stopped, layout stabilized.');
- }).width(window.innerWidth).height(window.innerHeight);
+ const el = document.getElementById('schema-graph');
+ const { width, height } = el.getBoundingClientRect();
+ Graph.onEngineStop(() => {
+ // console.debug('Schema graph: layout stabilized');
+ }).width(width).height(height);Additionally, add a resize observer after this block to keep the canvas responsive when the sidebar opens/resizes:
// keep graph sized to container
const ro = new ResizeObserver(entries => {
for (const entry of entries) {
const cr = entry.contentRect;
Graph.width(cr.width).height(cr.height);
}
});
ro.observe(el);🤖 Prompt for AI Agents
In api/static/js/modules/schema.js around lines 71 to 75, the graph is sized to
the viewport using window.innerWidth/innerHeight which causes overflow in the
sidebar; replace those calls with the sidebar container's bounding client rect
(use el.getBoundingClientRect().width/height) when setting Graph.width/height,
and after that block add a ResizeObserver that observes the container element
(el) and on entries sets Graph.width(cr.width).height(cr.height) to keep the
canvas responsive as the sidebar opens/resizes.
| export function toggleContainer(container, onOpen) { | ||
| // Check if we're on mobile (768px breakpoint to match CSS) | ||
| const isMobile = window.innerWidth <= 768; | ||
|
|
||
| if (!DOM.menuContainer.classList.contains('open')) { | ||
| DOM.menuContainer.classList.add('open'); | ||
| DOM.sideMenuButton.style.display = 'none'; | ||
|
|
||
| // find all the containers with class "sidebar-container" and if open remove open | ||
| const allContainers = document.querySelectorAll('.sidebar-container'); | ||
| allContainers.forEach((c) => { | ||
| if (c !== container && c.classList.contains('open')) { | ||
| c.classList.remove('open'); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against null/undefined container.
toggleContainer assumes a valid HTMLElement; if a caller passes null (e.g., missing markup), this will throw.
Apply this diff:
-export function toggleContainer(container, onOpen) {
+export function toggleContainer(container, onOpen) {
+ if (!container) {
+ console.warn('toggleContainer: no container provided');
+ return;
+ }📝 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 toggleContainer(container, onOpen) { | |
| // Check if we're on mobile (768px breakpoint to match CSS) | |
| const isMobile = window.innerWidth <= 768; | |
| if (!DOM.menuContainer.classList.contains('open')) { | |
| DOM.menuContainer.classList.add('open'); | |
| DOM.sideMenuButton.style.display = 'none'; | |
| // find all the containers with class "sidebar-container" and if open remove open | |
| const allContainers = document.querySelectorAll('.sidebar-container'); | |
| allContainers.forEach((c) => { | |
| if (c !== container && c.classList.contains('open')) { | |
| c.classList.remove('open'); | |
| } | |
| }); | |
| export function toggleContainer(container, onOpen) { | |
| if (!container) { | |
| console.warn('toggleContainer: no container provided'); | |
| return; | |
| } | |
| // Check if we're on mobile (768px breakpoint to match CSS) | |
| const isMobile = window.innerWidth <= 768; | |
| // find all the containers with class "sidebar-container" and if open remove open | |
| const allContainers = document.querySelectorAll('.sidebar-container'); | |
| allContainers.forEach((c) => { | |
| if (c !== container && c.classList.contains('open')) { | |
| c.classList.remove('open'); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In api/static/js/modules/ui.js around lines 7 to 18, toggleContainer assumes the
passed container is a valid HTMLElement and will throw if null/undefined; add an
early guard that checks the argument is non-null and is an Element/HTMLElement
before using it (e.g., if (!container || !(container instanceof Element))
return;), then proceed with the existing logic so callers that pass null or
missing markup no longer cause runtime errors.
api/static/js/modules/ui.js
Outdated
| export function setupToolbar() { | ||
| // Keyboard navigation: roving tabindex within #toolbar-buttons | ||
| const toolbar = document.getElementById('toolbar-buttons'); | ||
| if (toolbar) { | ||
| const buttons = Array.from(toolbar.querySelectorAll('button.toolbar-button')); | ||
| // Ensure first button is tabbable | ||
| buttons.forEach((b, i) => b.setAttribute('tabindex', i === 0 ? '0' : '-1')); | ||
|
|
||
| toolbar.addEventListener('keydown', (e) => { | ||
| const focused = document.activeElement; | ||
| const idx = buttons.indexOf(focused); | ||
| if (e.key === 'ArrowDown' || e.key === 'ArrowRight') { | ||
| e.preventDefault(); | ||
| const next = buttons[(idx + 1) % buttons.length]; | ||
| if (next) { | ||
| buttons.forEach(b => b.setAttribute('tabindex', '-1')); | ||
| next.setAttribute('tabindex', '0'); | ||
| next.focus(); | ||
| } | ||
| } else if (e.key === 'ArrowUp' || e.key === 'ArrowLeft') { | ||
| e.preventDefault(); | ||
| const prev = buttons[(idx - 1 + buttons.length) % buttons.length]; | ||
| if (prev) { | ||
| buttons.forEach(b => b.setAttribute('tabindex', '-1')); | ||
| prev.setAttribute('tabindex', '0'); | ||
| prev.focus(); | ||
| } | ||
| } else if (e.key === 'Enter' || e.key === ' ') { | ||
| // Activate the button | ||
| e.preventDefault(); | ||
| if (focused) focused.click(); | ||
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden toolbar keyboard navigation and use centralized DOM reference.
- Use DOM.toolbarButtons to keep selectors centralized.
- Add role="toolbar" and aria-orientation for a11y.
- Handle empty toolbar, idx === -1, Home/End keys.
- Keep roving tabindex in sync on click as well.
Apply this diff:
-export function setupToolbar() {
- // Keyboard navigation: roving tabindex within #toolbar-buttons
- const toolbar = document.getElementById('toolbar-buttons');
- if (toolbar) {
- const buttons = Array.from(toolbar.querySelectorAll('button.toolbar-button'));
- // Ensure first button is tabbable
- buttons.forEach((b, i) => b.setAttribute('tabindex', i === 0 ? '0' : '-1'));
-
- toolbar.addEventListener('keydown', (e) => {
- const focused = document.activeElement;
- const idx = buttons.indexOf(focused);
- if (e.key === 'ArrowDown' || e.key === 'ArrowRight') {
- e.preventDefault();
- const next = buttons[(idx + 1) % buttons.length];
- if (next) {
- buttons.forEach(b => b.setAttribute('tabindex', '-1'));
- next.setAttribute('tabindex', '0');
- next.focus();
- }
- } else if (e.key === 'ArrowUp' || e.key === 'ArrowLeft') {
- e.preventDefault();
- const prev = buttons[(idx - 1 + buttons.length) % buttons.length];
- if (prev) {
- buttons.forEach(b => b.setAttribute('tabindex', '-1'));
- prev.setAttribute('tabindex', '0');
- prev.focus();
- }
- } else if (e.key === 'Enter' || e.key === ' ') {
- // Activate the button
- e.preventDefault();
- if (focused) focused.click();
- }
- });
- }
-}
+export function setupToolbar() {
+ // Keyboard navigation: roving tabindex within #toolbar-buttons
+ const toolbar = DOM.toolbarButtons || document.getElementById('toolbar-buttons');
+ if (!toolbar) return;
+ const buttons = Array.from(toolbar.querySelectorAll('button.toolbar-button'));
+ if (buttons.length === 0) return;
+
+ toolbar.setAttribute('role', 'toolbar');
+ toolbar.setAttribute('aria-orientation', 'vertical');
+
+ const setActive = (el) => {
+ buttons.forEach(b => b.setAttribute('tabindex', '-1'));
+ if (el) el.setAttribute('tabindex', '0');
+ };
+ // Ensure first button is tabbable
+ setActive(buttons[0]);
+
+ // Keep roving tabindex in sync on click
+ buttons.forEach((b) => {
+ b.addEventListener('click', () => setActive(b));
+ });
+
+ toolbar.addEventListener('keydown', (e) => {
+ const focused = document.activeElement;
+ let idx = buttons.indexOf(focused);
+ if (idx === -1) idx = 0;
+ if (e.key === 'ArrowDown' || e.key === 'ArrowRight') {
+ e.preventDefault();
+ const next = buttons[(idx + 1) % buttons.length];
+ if (next) {
+ setActive(next);
+ next.focus();
+ }
+ } else if (e.key === 'ArrowUp' || e.key === 'ArrowLeft') {
+ e.preventDefault();
+ const prev = buttons[(idx - 1 + buttons.length) % buttons.length];
+ if (prev) {
+ setActive(prev);
+ prev.focus();
+ }
+ } else if (e.key === 'Home') {
+ e.preventDefault();
+ setActive(buttons[0]); buttons[0].focus();
+ } else if (e.key === 'End') {
+ e.preventDefault();
+ const last = buttons[buttons.length - 1];
+ setActive(last); last.focus();
+ } else if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ if (focused && typeof focused.click === 'function') focused.click();
+ }
+ });
+}| // Find all the containers with class "sidebar-container" and if open remove open | ||
| const allContainers = document.querySelectorAll('.sidebar-container'); | ||
| allContainers.forEach((c) => { | ||
| // If menu is open and we switch to mobile, remove any desktop padding | ||
| if (isMobile && c.classList.contains('open')) { | ||
| DOM.chatContainer.style.paddingRight = ''; | ||
| DOM.chatContainer.style.paddingLeft = ''; | ||
| } | ||
| // If menu is open and we switch to desktop, apply desktop padding | ||
| else if (!isMobile && c.classList.contains('open')) { | ||
| DOM.chatContainer.style.paddingRight = '10%'; | ||
| DOM.chatContainer.style.paddingLeft = '10%'; | ||
| } | ||
| // If menu is closed and we're on desktop, ensure default desktop padding | ||
| else if (!isMobile && !c.classList.contains('open')) { | ||
| DOM.chatContainer.style.paddingRight = '20%'; | ||
| DOM.chatContainer.style.paddingLeft = '20%'; | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Fix padding logic on resize: current loop can override with closed containers.
Iterating each container and setting chat padding per container results in the last iteration “winning”, so an open container can be effectively ignored if any closed container is encountered later. Compute once based on “any open” instead.
Apply this diff:
- // Find all the containers with class "sidebar-container" and if open remove open
- const allContainers = document.querySelectorAll('.sidebar-container');
- allContainers.forEach((c) => {
- // If menu is open and we switch to mobile, remove any desktop padding
- if (isMobile && c.classList.contains('open')) {
- DOM.chatContainer.style.paddingRight = '';
- DOM.chatContainer.style.paddingLeft = '';
- }
- // If menu is open and we switch to desktop, apply desktop padding
- else if (!isMobile && c.classList.contains('open')) {
- DOM.chatContainer.style.paddingRight = '10%';
- DOM.chatContainer.style.paddingLeft = '10%';
- }
- // If menu is closed and we're on desktop, ensure default desktop padding
- else if (!isMobile && !c.classList.contains('open')) {
- DOM.chatContainer.style.paddingRight = '20%';
- DOM.chatContainer.style.paddingLeft = '20%';
- }
- });
+ const allContainers = document.querySelectorAll('.sidebar-container');
+ const anyOpen = Array.from(allContainers).some(c => c.classList.contains('open'));
+
+ if (isMobile) {
+ DOM.chatContainer.style.paddingRight = '';
+ DOM.chatContainer.style.paddingLeft = '';
+ return;
+ }
+ if (anyOpen) {
+ DOM.chatContainer.style.paddingRight = '10%';
+ DOM.chatContainer.style.paddingLeft = '10%';
+ } else {
+ DOM.chatContainer.style.paddingRight = '20%';
+ DOM.chatContainer.style.paddingLeft = '20%';
+ }📝 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.
| // Find all the containers with class "sidebar-container" and if open remove open | |
| const allContainers = document.querySelectorAll('.sidebar-container'); | |
| allContainers.forEach((c) => { | |
| // If menu is open and we switch to mobile, remove any desktop padding | |
| if (isMobile && c.classList.contains('open')) { | |
| DOM.chatContainer.style.paddingRight = ''; | |
| DOM.chatContainer.style.paddingLeft = ''; | |
| } | |
| // If menu is open and we switch to desktop, apply desktop padding | |
| else if (!isMobile && c.classList.contains('open')) { | |
| DOM.chatContainer.style.paddingRight = '10%'; | |
| DOM.chatContainer.style.paddingLeft = '10%'; | |
| } | |
| // If menu is closed and we're on desktop, ensure default desktop padding | |
| else if (!isMobile && !c.classList.contains('open')) { | |
| DOM.chatContainer.style.paddingRight = '20%'; | |
| DOM.chatContainer.style.paddingLeft = '20%'; | |
| } | |
| }); | |
| const allContainers = document.querySelectorAll('.sidebar-container'); | |
| const anyOpen = Array.from(allContainers).some(c => c.classList.contains('open')); | |
| if (isMobile) { | |
| DOM.chatContainer.style.paddingRight = ''; | |
| DOM.chatContainer.style.paddingLeft = ''; | |
| return; | |
| } | |
| if (anyOpen) { | |
| DOM.chatContainer.style.paddingRight = '10%'; | |
| DOM.chatContainer.style.paddingLeft = '10%'; | |
| } else { | |
| DOM.chatContainer.style.paddingRight = '20%'; | |
| DOM.chatContainer.style.paddingLeft = '20%'; | |
| } |
🤖 Prompt for AI Agents
In api/static/js/modules/ui.js around lines 230 to 249, the current loop updates
DOM.chatContainer padding for every sidebar-container so later iterations can
override earlier ones; instead determine whether any container is open once
(e.g., Array.from(document.querySelectorAll('.sidebar-container')).some(c =>
c.classList.contains('open'))) and then set padding once based on isMobile and
that single boolean: clear padding when mobile, apply 10% when desktop and any
open, and apply 20% when desktop and none open.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
api/static/js/modules/schema.js (3)
17-19: Guard against non-array columns in renderingEven if the backend normally supplies an array, guard client-side to prevent runtime errors.
Apply this diff:
- // compute box height from number of columns - const boxHeight = headerHeight + node.columns.length * lineHeight + padding * 2; + // compute box height from number of columns (defensive) + const columns = Array.isArray(node.columns) ? node.columns : []; + const boxHeight = headerHeight + columns.length * lineHeight + padding * 2; @@ - node.columns.forEach((col) => { + columns.forEach((col) => {Also applies to: 39-75
132-166: Size the canvas to the sidebar container and keep it responsiveSizing to the window causes overflow in the sidebar; read container bounds and observe resizes.
Apply this diff:
- Graph.onEngineStop(() => { - console.log('Engine stopped, layout stabilized.'); - }).width(window.innerWidth).height(window.innerHeight); + const el = document.getElementById('schema-graph'); + const { width, height } = el.getBoundingClientRect(); + Graph.onEngineStop(() => { + // layout stabilized + }).width(width).height(height); + + // keep graph sized to container + try { + const ro = new ResizeObserver(entries => { + for (const entry of entries) { + const cr = entry.contentRect; + Graph.width(cr.width).height(cr.height); + } + }); + ro.observe(el); + } catch (e) { + // ResizeObserver not available; ignore + } @@ }
78-113: Prevent early-tick crashes when link endpoints aren’t resolved yetForceGraph can pass links whose source/target haven’t been resolved to positioned node objects; accessing x/y can throw.
Apply this diff:
const edgeColor = getEdgeColor(); ctx.strokeStyle = edgeColor; ctx.lineWidth = 1.5; - ctx.beginPath(); - ctx.moveTo(link.source.x, link.source.y); - ctx.lineTo(link.target.x, link.target.y); - ctx.stroke(); + if (!link.source || !link.target || link.source.x == null || link.target.x == null) return; + ctx.beginPath(); + ctx.moveTo(link.source.x, link.source.y); + ctx.lineTo(link.target.x, link.target.y); + ctx.stroke();
🧹 Nitpick comments (4)
api/routes/graphs.py (1)
71-74: Docstring mismatch: you return “links”, not “edges”The docstring says edges, but the response uses links. Align wording to avoid confusion.
Apply this diff:
- This endpoint returns a JSON object with two keys: `nodes` and `edges`. - Nodes contain a minimal set of properties (id, name, labels, props). - Edges contain source and target node names (or internal ids), type and props. + This endpoint returns a JSON object with two keys: `nodes` and `links`. + Nodes contain a minimal set of properties (id, name, columns). + Links contain source and target table names.api/static/js/modules/schema.js (2)
4-6: Be explicit about node identity to future-proof link resolutionForceGraph defaults to id='id', but make it explicit to avoid regressions if data shape changes.
Apply this diff:
- const Graph = ForceGraph()(document.getElementById('schema-graph')) - .graphData(data) + const Graph = ForceGraph()(document.getElementById('schema-graph')) + .nodeId('id') + .graphData(data)
166-166: Return the Graph instance for callers who want to customize or teardownReturning the instance makes testing and future composition easier.
Apply this diff:
-} + return Graph; +}api/static/js/app.js (1)
22-49: Consider aborting stale fetches to avoid race conditions on rapid switchesIf users flip graphs quickly, earlier responses can race and overwrite newer renders. Abort previous requests.
Apply this diff to the function:
async function loadAndShowGraph(selected) { if (!selected) { return; } try { - const resp = await fetch(`/graphs/${encodeURIComponent(selected)}/data`); + // Abort any in-flight schema fetch + if (window.__schemaFetchCtl) { + try { window.__schemaFetchCtl.abort(); } catch (e) {} + } + const ctl = new AbortController(); + window.__schemaFetchCtl = ctl; + const resp = await fetch(`/graphs/${encodeURIComponent(selected)}/data`, { signal: ctl.signal });Optionally, declare a module-scoped controller instead of using window:
let currentSchemaFetchController;and replace window.__schemaFetchCtl with currentSchemaFetchController.
</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **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. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4fc937007242f8cf46cfb24609a7d8a145dffb24 and 06e15c98c3d05ffcd9a171ee1568cdf11f6af5a3. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `api/routes/graphs.py` (1 hunks) * `api/static/js/app.js` (4 hunks) * `api/static/js/modules/schema.js` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (3)</summary> <details> <summary>api/static/**</summary> **📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)** > Store frontend static assets under api/static/ Files: - `api/static/js/modules/schema.js` - `api/static/js/app.js` </details> <details> <summary>api/routes/graphs.py</summary> **📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)** > Keep graph/database route handlers in api/routes/graphs.py Files: - `api/routes/graphs.py` </details> <details> <summary>api/routes/**/*.py</summary> **📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)** > Place all additional Flask route handlers under api/routes/ Files: - `api/routes/graphs.py` </details> </details><details> <summary>🧬 Code Graph Analysis (3)</summary> <details> <summary>api/static/js/modules/schema.js (1)</summary><blockquote> <details> <summary>api/static/js/app.js (1)</summary> * `data` (34-34) </details> </blockquote></details> <details> <summary>api/routes/graphs.py (1)</summary><blockquote> <details> <summary>api/auth/user_management.py (1)</summary> * `token_required` (263-285) </details> </blockquote></details> <details> <summary>api/static/js/app.js (4)</summary><blockquote> <details> <summary>api/static/js/modules/schema.js (1)</summary> * `showGraph` (2-166) </details> <details> <summary>api/static/js/modules/config.js (2)</summary> * `DOM` (39-65) * `DOM` (39-65) </details> <details> <summary>api/static/js/modules/ui.js (2)</summary> * `toggleContainer` (7-39) * `setupToolbar` (141-225) </details> <details> <summary>api/static/js/modules/graphs.js (1)</summary> * `onGraphChange` (115-117) </details> </blockquote></details> </details><details> <summary>🪛 GitHub Check: CodeQL</summary> <details> <summary>api/routes/graphs.py</summary> [failure] 84-84: Log Injection This log entry depends on a [user-provided value](1). --- [failure] 105-105: Log Injection This log entry depends on a [user-provided value](1). </details> </details> <details> <summary>🪛 GitHub Actions: Pylint</summary> <details> <summary>api/routes/graphs.py</summary> [warning] 40-40: Pylint: Unnecessary 'elif' after 'return'. (no-else-return). --- [warning] 68-68: Pylint: Too many local variables (21/15). --- [warning] 83-83: Pylint: Broad exception caught. (broad-exception-caught). --- [warning] 104-104: Pylint: Broad exception caught. (broad-exception-caught). --- [warning] 112-112: Pylint: Broad exception caught. (broad-exception-caught). --- [warning] 140-140: Pylint: Broad exception caught. (broad-exception-caught). --- [warning] 154-154: Pylint: Broad exception caught. (broad-exception-caught). --- [warning] 68-68: Pylint: Too many branches (16/12). --- [warning] 68-68: Pylint: Too many statements (52/50). --- [warning] 167-167: Pylint: Too many return statements (8/6). --- [warning] 278-278: Pylint: Too many local variables (28/15). --- [warning] 324-324: Pylint: Broad exception caught. (broad-exception-caught). --- [warning] 478-478: Pylint: Broad exception caught. (broad-exception-caught). --- [warning] 278-278: Pylint: Too many branches (18/12). --- [warning] 278-278: Pylint: Too many statements (78/50). --- [warning] 289-289: Pylint: Unused variable 'db_type' (unused-variable). --- [warning] 246-246: Pylint: Too many statements (90/50). --- [warning] 586-586: Pylint: Broad exception caught. (broad-exception-caught). --- [warning] 509-509: Pylint: Unused variable 'db_type' (unused-variable). --- [warning] 647-647: Pylint: Broad exception caught. (broad-exception-caught). </details> </details> </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>api/routes/graphs.py (3)</summary> `66-74`: **Nice addition: user-scoped, authenticated graph-data endpoint** Route shape, auth, and namespacing look good and align with the frontend fetch. --- `108-148`: **Robust column normalization is solid** Tolerant of mixed driver return shapes and safely skips malformed entries. Good defensive coding. --- `149-162`: **Link deduplication and shape look good** Dedup via set and return {source, target} matches frontend expectations. </details> <details> <summary>api/static/js/app.js (2)</summary> `76-91`: **Good UX: fetch on panel open and size the work to current selection** Wires schema panel open to data load correctly and avoids unnecessary fetches when nothing is selected. --- `112-124`: **Nice: refresh graph view when the selection changes while panel is open** Preserves existing behavior and adds the right conditional fetch. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| try: | ||
| tables_res = graph.query(tables_query).result_set | ||
| links_res = graph.query(links_query).result_set | ||
| except Exception as e: | ||
| logging.error("Error querying graph data for %s: %s", namespaced, e) | ||
| return jsonify({"error": "Failed to read graph data"}), 500 | ||
|
|
There was a problem hiding this comment.
Sanitize log entry and keep stack trace (CodeQL: Log Injection)
Use %r for the user-controlled name and include exc_info for debugging.
Apply this diff:
- except Exception as e:
- logging.error("Error querying graph data for %s: %s", namespaced, e)
+ except Exception as e:
+ logging.error("Error querying graph data for %r: %s", namespaced, e, exc_info=True)
return jsonify({"error": "Failed to read graph data"}), 500🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 105-105: Log Injection
This log entry depends on a user-provided value.
🪛 GitHub Actions: Pylint
[warning] 104-104: Pylint: Broad exception caught. (broad-exception-caught).
🤖 Prompt for AI Agents
In api/routes/graphs.py around lines 101 to 107, the logging.error call logs a
user-controlled value directly and omits the exception stack trace; change the
log to use a safe representation (%r) for the namespaced value and pass
exc_info=True to include the exception traceback (e.g., logging.error("Error
querying graph data for %r", namespaced, exc_info=True, stack_info=False)) so
the message is sanitized and full error context is retained; keep the existing
JSON error response unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
api/routes/graphs.py (1)
84-85: Fix CodeQL "Log Injection" finding: sanitize user-controlled data in logsFormat with %r to escape control characters and prevent log forging. This was flagged earlier and remains unfixed.
- logging.error("Failed to select graph %s: %s", namespaced, e) + logging.error("Failed to select graph %r: %r", namespaced, e) @@ - logging.error("Error querying graph data for %s: %s", namespaced, e) + logging.error("Error querying graph data for %r: %r", namespaced, e)Also applies to: 105-106
api/static/js/modules/schema.js (4)
17-19: Defensive rendering: guard when node.columns is missing or not an arrayAvoids runtime errors if data is malformed or in future changes. This was raised previously.
- // compute box height from number of columns - const boxHeight = headerHeight + node.columns.length * lineHeight + padding * 2; + // compute box height from number of columns (defensive) + const columns = Array.isArray(node.columns) ? node.columns : []; + const boxHeight = headerHeight + columns.length * lineHeight + padding * 2;
39-75: Use guarded columns array for iteration and width calculationsFollow-up to the guard above; iterate over the safe columns array to avoid exceptions.
- node.columns.forEach((col) => { + columns.forEach((col) => { let name = col; let type = null; if (typeof col === 'object') { name = col.name || ''; type = col.type || col.dataType || null; } @@ - const nameWidth = ctx.measureText(name).width; + const nameWidth = ctx.measureText(name).width;
132-136: Size canvas to container instead of viewport; add resize observerThe schema graph lives in a sidebar; sizing to window causes overflow and layout issues. Use the container box. Previously suggested.
- Graph.onEngineStop(() => { - console.log('Engine stopped, layout stabilized.'); - }).width(window.innerWidth).height(window.innerHeight); + const el = document.getElementById('schema-graph'); + const { width, height } = el.getBoundingClientRect(); + Graph.onEngineStop(() => { + // layout stabilized + }).width(width).height(height);Add this right after the block above to keep it responsive:
// keep graph sized to container const ro = new ResizeObserver(entries => { for (const entry of entries) { const cr = entry.contentRect; Graph.width(cr.width).height(cr.height); } }); ro.observe(document.getElementById('schema-graph'));
78-113: Prevent early-tick crashes when link positions are undefined; improve robustnessDuring early ticks link.source/target may be unresolved or lack x/y. Guard before drawing. This was suggested earlier.
- .linkCanvasObject((link, ctx) => { + .linkCanvasObject((link, ctx) => { const getEdgeColor = () => { @@ const edgeColor = getEdgeColor(); ctx.strokeStyle = edgeColor; ctx.lineWidth = 1.5; ctx.beginPath(); - ctx.moveTo(link.source.x, link.source.y); - ctx.lineTo(link.target.x, link.target.y); + if (!link?.source || !link?.target || + link.source.x == null || link.source.y == null || + link.target.x == null || link.target.y == null) { + return; + } + ctx.moveTo(link.source.x, link.source.y); + ctx.lineTo(link.target.x, link.target.y); ctx.stroke(); })
🧹 Nitpick comments (7)
api/routes/graphs.py (2)
71-74: Docstring mismatch: returns "links", not "edges"The API returns {"nodes": [...], "links": [...]}, but the docstring says "edges". Align to avoid confusion and to match the frontend expectations.
- This endpoint returns a JSON object with two keys: `nodes` and `edges`. - Nodes contain a minimal set of properties (id, name, labels, props). - Edges contain source and target node names (or internal ids), type and props. + This endpoint returns a JSON object with two keys: `nodes` and `links`. + Nodes contain: id, name, and columns (list of {name, type|null}). + Links contain: source and target table names (deduplicated).
75-80: Harden graph_id and keep length limits consistent with other routes
- Align the max length with query_graph (which trims to 100 chars at Line 255).
- Add a conservative character whitelist to avoid surprising graph names and reduce risk of log forging or path traversal in downstream systems.
- graph_id = graph_id.strip()[:200] + graph_id = graph_id.strip()[:100] + # allow only alphanumerics, underscore and hyphen + if not all(c.isalnum() or c in ('_', '-') for c in graph_id): + return jsonify({"error": "Invalid graph_id"}), 400 namespaced = g.user_id + "_" + graph_idapi/static/js/modules/schema.js (1)
4-6: Set nodeId explicitly to 'id' to match link force mappingYou’re already using d3.forceLink().id(d => d.id). Make the Graph’s nodeId explicit and consistent to remove ambiguity and future regressions.
- const Graph = ForceGraph()(document.getElementById('schema-graph')) - .graphData(data) + const Graph = ForceGraph()(document.getElementById('schema-graph')) + .nodeId('id') + .graphData(data)api/static/js/app.js (4)
28-29: Minor: hint desired response type to the serverNot required, but explicitly requesting JSON is a harmless clarity/interop improvement.
- const resp = await fetch(`/graphs/${encodeURIComponent(selected)}/data`); + const resp = await fetch(`/graphs/${encodeURIComponent(selected)}/data`, { + headers: { 'Accept': 'application/json' } + });
36-39: Clear schema container on invalid data to avoid stale graph viewIf shape is invalid, clear the container so users don’t see an outdated graph.
- if (!data || !Array.isArray(data.nodes) || !Array.isArray(data.links)) { - console.warn('Graph data returned in unexpected shape, showing empty message', data); - return; - } + if (!data || !Array.isArray(data.nodes) || !Array.isArray(data.links)) { + console.warn('Graph data returned in unexpected shape, clearing view', data); + const container = document.getElementById('schema-graph'); + if (container) container.innerHTML = ''; + return; + }
46-49: Also clear schema container when fetch failsKeeps UI consistent when network/parse errors occur.
} catch (err) { console.error('Error fetching graph data:', err); + const container = document.getElementById('schema-graph'); + if (container) container.innerHTML = ''; }
112-124: When selection is cleared and schema panel is open, clear the graphPrevents showing a graph from a previously selected database when no selection is active.
DOM.graphSelect.addEventListener("change", async () => { // Preserve existing behavior onGraphChange(); // If the schema panel is currently open, fetch and render the selected graph const selected = DOM.graphSelect.value; - if (!selected) return; - - if (DOM.schemaContainer && DOM.schemaContainer.classList.contains('open')) { - await loadAndShowGraph(selected); - } + if (DOM.schemaContainer && DOM.schemaContainer.classList.contains('open')) { + if (!selected) { + const container = document.getElementById('schema-graph'); + if (container) container.innerHTML = ''; + return; + } + await loadAndShowGraph(selected); + } });
📜 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 (3)
api/routes/graphs.py(1 hunks)api/static/js/app.js(4 hunks)api/static/js/modules/schema.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
api/routes/graphs.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Keep graph/database route handlers in api/routes/graphs.py
Files:
api/routes/graphs.py
api/routes/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Place all additional Flask route handlers under api/routes/
Files:
api/routes/graphs.py
api/static/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Store frontend static assets under api/static/
Files:
api/static/js/modules/schema.jsapi/static/js/app.js
🧬 Code Graph Analysis (3)
api/routes/graphs.py (1)
api/auth/user_management.py (1)
token_required(263-285)
api/static/js/modules/schema.js (1)
api/static/js/app.js (1)
data(34-34)
api/static/js/app.js (4)
api/static/js/modules/schema.js (1)
showGraph(2-166)api/static/js/modules/config.js (2)
DOM(39-65)DOM(39-65)api/static/js/modules/ui.js (2)
toggleContainer(7-39)setupToolbar(141-225)api/static/js/modules/graphs.js (1)
onGraphChange(115-117)
🪛 GitHub Check: CodeQL
api/routes/graphs.py
[failure] 84-84: Log Injection
This log entry depends on a user-provided value.
[failure] 105-105: Log Injection
This log entry depends on a user-provided value.
| try: | ||
| graph = db.select_graph(namespaced) | ||
| except Exception as e: | ||
| logging.error("Failed to select graph %s: %s", namespaced, e) | ||
| return jsonify({"error": "Graph not found or database error"}), 404 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing graph object explicitly after selection
If db.select_graph returns a falsy object instead of throwing, we should respond with 404. This avoids attribute errors when accessing graph.query.
try:
graph = db.select_graph(namespaced)
except Exception as e:
logging.error("Failed to select graph %s: %s", namespaced, e)
return jsonify({"error": "Graph not found or database error"}), 404
+ if not graph:
+ return jsonify({"error": "Graph not found"}), 404📝 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: | |
| graph = db.select_graph(namespaced) | |
| except Exception as e: | |
| logging.error("Failed to select graph %s: %s", namespaced, e) | |
| return jsonify({"error": "Graph not found or database error"}), 404 | |
| try: | |
| graph = db.select_graph(namespaced) | |
| except Exception as e: | |
| logging.error("Failed to select graph %s: %s", namespaced, e) | |
| return jsonify({"error": "Graph not found or database error"}), 404 | |
| if not graph: | |
| return jsonify({"error": "Graph not found"}), 404 |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 84-84: Log Injection
This log entry depends on a user-provided value.
🤖 Prompt for AI Agents
In api/routes/graphs.py around lines 81 to 86, the code only handles exceptions
from db.select_graph but not the case where it returns a falsy value; add an
explicit check after the try/except: if graph is None or falsy, log an
informative message including the namespaced id and return a 404 JSON response
so subsequent access like graph.query won't raise an AttributeError.
| try: | ||
| graph = db.select_graph(namespaced) | ||
| except Exception as e: | ||
| logging.error("Failed to select graph %s: %s", sanitize_log_input(namespaced), e) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the log injection vulnerability, we should improve the sanitize_log_input function to remove or escape all control characters that could affect log formatting, not just newlines and carriage returns. This includes tab (\t), form feed (\f), vertical tab (\v), and Unicode line/paragraph separators. The function should also clearly mark user input in the log entry, for example by quoting it. The best way to do this is to use a regular expression to remove all characters in the Unicode "control" category, or to replace them with safe alternatives (such as spaces). The change should be made in the definition of sanitize_log_input in api/routes/graphs.py. No changes to the logging calls themselves are needed if the sanitization is robust.
Required changes:
- Update the
sanitize_log_inputfunction to remove all control characters using a regular expression. - Add the required import for
re(the regular expression module).
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import json | ||
| import logging | ||
| import re | ||
| from concurrent.futures import ThreadPoolExecutor | ||
| from concurrent.futures import TimeoutError as FuturesTimeoutError | ||
|
|
||
| @@ -50,10 +51,13 @@ | ||
| return query.replace('\n', ' ').replace('\r', ' ')[:500] | ||
|
|
||
| def sanitize_log_input(value: str) -> str: | ||
| """Sanitize input for safe logging (remove newlines and carriage returns).""" | ||
| """Sanitize input for safe logging (remove all control characters and clearly mark user input).""" | ||
| if not isinstance(value, str): | ||
| return str(value) | ||
| return value.replace('\n', ' ').replace('\r', ' ') | ||
| value = str(value) | ||
| # Remove all Unicode control characters (category Cc) | ||
| value = re.sub(r'[\x00-\x1F\x7F-\x9F]', ' ', value) | ||
| # Optionally, quote the user input to make it clear in logs | ||
| return f'"{value}"' | ||
|
|
||
| @graphs_bp.route("") | ||
| @token_required |
| tables_res = graph.query(tables_query).result_set | ||
| links_res = graph.query(links_query).result_set | ||
| except Exception as e: | ||
| logging.error("Error querying graph data for %s: %s", sanitize_log_input(namespaced), e) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the log injection vulnerability, we should enhance the sanitize_log_input function to remove all control characters (not just newlines and carriage returns) from user input before logging. This can be done by using a regular expression to remove all characters with ASCII codes below 0x20 (except for space), which includes tabs and other control characters. Additionally, we should clearly mark user input in the log entry to prevent confusion. The changes should be made in the sanitize_log_input function in api/routes/graphs.py, and all calls to this function will benefit from the improved sanitization.
| @@ -49,11 +49,14 @@ | ||
| """Sanitize the query to prevent injection attacks.""" | ||
| return query.replace('\n', ' ').replace('\r', ' ')[:500] | ||
|
|
||
| import re | ||
|
|
||
| def sanitize_log_input(value: str) -> str: | ||
| """Sanitize input for safe logging (remove newlines and carriage returns).""" | ||
| """Sanitize input for safe logging (remove control characters).""" | ||
| if not isinstance(value, str): | ||
| return str(value) | ||
| return value.replace('\n', ' ').replace('\r', ' ') | ||
| value = str(value) | ||
| # Remove all ASCII control characters except space (0x20) | ||
| return re.sub(r'[\x00-\x1F\x7F]', '', value) | ||
|
|
||
| @graphs_bp.route("") | ||
| @token_required |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
api/static/css/menu.css (1)
166-179: Schema graph selector mismatch fixed; consider a temporary fallback to #graphGood catch switching the container to #schema-graph and ensuring the canvas fills it. If any legacy templates still reference id="graph", consider including both selectors for a short transition period to avoid unexpected regressions.
-#schema-graph { +#schema-graph, +#graph { width: 100%; height: 60vh; /* allow graph to take most of the sidebar height */ min-height: 300px; background: transparent; position: relative; } - .force-graph-container, #schema-graph > canvas { + .force-graph-container, + #schema-graph > canvas, + #graph > canvas { width: 100% !important; height: 100% !important; }You can verify template usage with:
#!/bin/bash rg -nC2 'id="schema-graph"|id="graph"' -g 'api/templates/**' -Sapi/static/js/modules/ui.js (3)
141-175: Harden toolbar keyboard navigation and improve a11y semanticsAdd role/aria attributes, handle empty toolbars, Home/End keys, and keep roving tabindex in sync on click. This improves accessibility and robustness.
-export function setupToolbar() { - // Keyboard navigation: roving tabindex within #toolbar-buttons - const toolbar = document.getElementById('toolbar-buttons'); - if (toolbar) { - const buttons = Array.from(toolbar.querySelectorAll('button.toolbar-button')); - // Ensure first button is tabbable - buttons.forEach((b, i) => b.setAttribute('tabindex', i === 0 ? '0' : '-1')); - - toolbar.addEventListener('keydown', (e) => { - const focused = document.activeElement; - const idx = buttons.indexOf(focused); - if (e.key === 'ArrowDown' || e.key === 'ArrowRight') { - e.preventDefault(); - const next = buttons[(idx + 1) % buttons.length]; - if (next) { - buttons.forEach(b => b.setAttribute('tabindex', '-1')); - next.setAttribute('tabindex', '0'); - next.focus(); - } - } else if (e.key === 'ArrowUp' || e.key === 'ArrowLeft') { - e.preventDefault(); - const prev = buttons[(idx - 1 + buttons.length) % buttons.length]; - if (prev) { - buttons.forEach(b => b.setAttribute('tabindex', '-1')); - prev.setAttribute('tabindex', '0'); - prev.focus(); - } - } else if (e.key === 'Enter' || e.key === ' ') { - // Activate the button - e.preventDefault(); - if (focused) focused.click(); - } - }); - } -} +export function setupToolbar() { + const toolbar = (typeof DOM !== 'undefined' && DOM.toolbarButtons) || document.getElementById('toolbar-buttons'); + if (!toolbar) return; + const buttons = Array.from(toolbar.querySelectorAll('button.toolbar-button')); + if (buttons.length === 0) return; + + toolbar.setAttribute('role', 'toolbar'); + toolbar.setAttribute('aria-orientation', 'vertical'); + + const setActive = (el) => { + buttons.forEach(b => b.setAttribute('tabindex', '-1')); + if (el) el.setAttribute('tabindex', '0'); + }; + setActive(buttons[0]); + + buttons.forEach((b) => b.addEventListener('click', () => setActive(b))); + + toolbar.addEventListener('keydown', (e) => { + const focused = document.activeElement; + let idx = buttons.indexOf(focused); + if (idx === -1) idx = 0; + if (e.key === 'ArrowDown' || e.key === 'ArrowRight') { + e.preventDefault(); + const next = buttons[(idx + 1) % buttons.length]; + setActive(next); next.focus(); + } else if (e.key === 'ArrowUp' || e.key === 'ArrowLeft') { + e.preventDefault(); + const prev = buttons[(idx - 1 + buttons.length) % buttons.length]; + setActive(prev); prev.focus(); + } else if (e.key === 'Home') { + e.preventDefault(); + setActive(buttons[0]); buttons[0].focus(); + } else if (e.key === 'End') { + e.preventDefault(); + const last = buttons[buttons.length - 1]; + setActive(last); last.focus(); + } else if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + if (focused && typeof focused.click === 'function') focused.click(); + } + }); +}
7-17: Guard against null/invalid container to avoid runtime errorstoggleContainer assumes a valid element. Add a defensive guard so missing markup or bad calls don’t throw.
-export function toggleContainer(container, onOpen) { +export function toggleContainer(container, onOpen) { + if (!container || !(container instanceof Element)) { + console.warn('toggleContainer: no valid container provided'); + return; + }
177-201: Fix chat padding logic on resize: current loop can override with closed containersSet padding once based on whether any sidebar is open, instead of per-container. Prevents later iterations from overriding an open state.
- // Find all the containers with class "sidebar-container" and if open remove open - const allContainers = document.querySelectorAll('.sidebar-container'); - allContainers.forEach((c) => { - // If menu is open and we switch to mobile, remove any desktop padding - if (isMobile && c.classList.contains('open')) { - DOM.chatContainer.style.paddingRight = ''; - DOM.chatContainer.style.paddingLeft = ''; - } - // If menu is open and we switch to desktop, apply desktop padding - else if (!isMobile && c.classList.contains('open')) { - DOM.chatContainer.style.paddingRight = '10%'; - DOM.chatContainer.style.paddingLeft = '10%'; - } - // If menu is closed and we're on desktop, ensure default desktop padding - else if (!isMobile && !c.classList.contains('open')) { - DOM.chatContainer.style.paddingRight = '20%'; - DOM.chatContainer.style.paddingLeft = '20%'; - } - }); + const allContainers = document.querySelectorAll('.sidebar-container'); + const anyOpen = Array.from(allContainers).some(c => c.classList.contains('open')); + if (isMobile) { + DOM.chatContainer.style.paddingRight = ''; + DOM.chatContainer.style.paddingLeft = ''; + return; + } + if (anyOpen) { + DOM.chatContainer.style.paddingRight = '10%'; + DOM.chatContainer.style.paddingLeft = '10%'; + } else { + DOM.chatContainer.style.paddingRight = '20%'; + DOM.chatContainer.style.paddingLeft = '20%'; + }api/routes/graphs.py (1)
107-113: Include traceback and safe formatting when query failsUse %r for the namespaced id and include exc_info for better diagnostics. This also addresses the CodeQL log-injection alert.
- except Exception as e: - logging.error("Error querying graph data for %s: %s", sanitize_log_input(namespaced), e) - return jsonify({"error": "Failed to read graph data"}), 500 + except Exception as e: + logging.error("Error querying graph data for %r: %s", namespaced, e, exc_info=True) + return jsonify({"error": "Failed to read graph data"}), 500api/static/js/modules/schema.js (3)
18-20: Defensive rendering for columns to prevent crashes when data is missingnode.columns is assumed to be an array; if it’s missing or not an array, this will throw. Default to an empty array for safe rendering.
- // compute box height from number of columns - const boxHeight = headerHeight + node.columns.length * lineHeight + padding * 2; + // compute box height from number of columns (defensive) + const columns = Array.isArray(node.columns) ? node.columns : []; + const boxHeight = headerHeight + columns.length * lineHeight + padding * 2; @@ - node.columns.forEach((col) => { + columns.forEach((col) => {Also applies to: 40-76
79-114: Guard link coordinates during early ticks to avoid canvas errors and improve contrastlink.source/target may be objects without x/y during early ticks. Add a guard to skip until positions exist. Consider a neutral gray for better contrast on light themes; your dynamic color logic is good, but still guard coordinates.
- .linkCanvasObject((link, ctx) => { + .linkCanvasObject((link, ctx) => { + if (!link?.source || !link?.target || + link.source.x == null || link.source.y == null || + link.target.x == null || link.target.y == null) { + return; + } const getEdgeColor = () => { try { const root = getComputedStyle(document.documentElement); // Prefer explicit CSS variable if present const cssEdge = root.getPropertyValue('--edge-color'); if (cssEdge && cssEdge.trim()) return cssEdge.trim(); @@ - return '#ffffff'; + return '#999'; }; const edgeColor = getEdgeColor(); ctx.strokeStyle = edgeColor; ctx.lineWidth = 1.5; ctx.beginPath(); ctx.moveTo(link.source.x, link.source.y); ctx.lineTo(link.target.x, link.target.y); ctx.stroke(); })
133-137: Size the canvas to the container, not the window, and keep it responsiveSizing to window spills beyond the sidebar panel. Use the container’s bounding box and a ResizeObserver to keep in sync when the sidebar opens/resizes.
- Graph.onEngineStop(() => { - console.log('Engine stopped, layout stabilized.'); - }).width(window.innerWidth).height(window.innerHeight); + const bb = el.getBoundingClientRect(); + Graph.onEngineStop(() => { + // layout stabilized + }).width(bb.width).height(bb.height); + + // Keep graph sized to container + const ro = new ResizeObserver(entries => { + for (const entry of entries) { + const cr = entry.contentRect; + Graph.width(cr.width).height(cr.height); + } + }); + ro.observe(el);
🧹 Nitpick comments (2)
api/static/css/menu.css (1)
3-17: Animate the left offset or use margin to avoid a visual jump when opening.left: 48px is applied only in the open state and isn’t part of the transition list, so it “jumps” into place. Either add left to the transition and set a closed-state baseline (left: 0) or, alternatively, use margin-left which is already part of the box model and feels more natural with layout.
.sidebar-container { width: 0; min-width: 0; overflow: hidden; padding: 20px 0; display: flex; flex-direction: column; gap: 20px; background: var(--falkor-quaternary); - transition: width 0.4s cubic-bezier(0.4, 0, 0.2, 1), padding 0.4s cubic-bezier(0.4, 0, 0.2, 1); + transition: width 0.4s cubic-bezier(0.4, 0, 0.2, 1), + padding 0.4s cubic-bezier(0.4, 0, 0.2, 1), + left 0.4s cubic-bezier(0.4, 0, 0.2, 1); z-index: 1; position: relative; + left: 0; /* baseline for smooth transition */ visibility: hidden; pointer-events: none; } .sidebar-container.open { width: 30dvw; padding: 20px; left: 48px; visibility: visible; pointer-events: auto; }Also applies to: 19-25
api/routes/graphs.py (1)
77-80: Docstring/property mismatch: returns "links", not "edges"The docstring mentions edges, but the response payload uses links. Align to avoid confusion for API consumers.
- This endpoint returns a JSON object with two keys: `nodes` and `edges`. + This endpoint returns a JSON object with two keys: `nodes` and `links`. - Edges contain source and target node names (or internal ids), type and props. + Links contain source and target table names and optional properties.
📜 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 (5)
api/routes/graphs.py(2 hunks)api/static/css/buttons.css(2 hunks)api/static/css/menu.css(3 hunks)api/static/js/modules/schema.js(1 hunks)api/static/js/modules/ui.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/static/css/buttons.css
🧰 Additional context used
📓 Path-based instructions (3)
api/static/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Store frontend static assets under api/static/
Files:
api/static/js/modules/schema.jsapi/static/js/modules/ui.jsapi/static/css/menu.css
api/routes/graphs.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Keep graph/database route handlers in api/routes/graphs.py
Files:
api/routes/graphs.py
api/routes/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Place all additional Flask route handlers under api/routes/
Files:
api/routes/graphs.py
🧬 Code Graph Analysis (3)
api/static/js/modules/schema.js (1)
api/static/js/app.js (1)
data(34-34)
api/routes/graphs.py (1)
api/auth/user_management.py (1)
token_required(263-285)
api/static/js/modules/ui.js (1)
api/static/js/modules/config.js (2)
DOM(39-65)DOM(39-65)
🪛 GitHub Check: CodeQL
api/routes/graphs.py
[failure] 90-90: Log Injection
This log entry depends on a user-provided value.
[failure] 111-111: Log Injection
This log entry depends on a user-provided value.
⏰ 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). (3)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
api/static/js/modules/schema.js (1)
119-128: Link force id accessor correctly matches nodeIdUsing d => d.name aligns with Graph.nodeId('name') and your data model. Good.
| try: | ||
| graph = db.select_graph(namespaced) | ||
| except Exception as e: | ||
| logging.error("Failed to select graph %s: %s", sanitize_log_input(namespaced), e) | ||
| return jsonify({"error": "Graph not found or database error"}), 404 | ||
|
|
There was a problem hiding this comment.
Handle missing graph object and harden logging (avoid None deref + log forging finding)
- db.select_graph can return None; calling graph.query would raise. Return 404 early.
- Use %r and exc_info=True in logs to mitigate log injection findings and preserve stack traces.
- Consider 500 for select_graph exceptions (DB errors) vs 404 for “not found”.
try:
- graph = db.select_graph(namespaced)
- except Exception as e:
- logging.error("Failed to select graph %s: %s", sanitize_log_input(namespaced), e)
- return jsonify({"error": "Graph not found or database error"}), 404
+ graph = db.select_graph(namespaced)
+ except Exception as e:
+ logging.error("Failed to select graph %r: %s", namespaced, e, exc_info=True)
+ return jsonify({"error": "Database error selecting graph"}), 500
+ if not graph:
+ logging.info("Graph not found: %r", namespaced)
+ return jsonify({"error": "Graph not found"}), 404Also applies to: 93-106
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 90-90: Log Injection
This log entry depends on a user-provided value.
🤖 Prompt for AI Agents
In api/routes/graphs.py around lines 87-92 (and similarly for 93-106),
db.select_graph may return None or raise an exception; update the flow to first
handle a successful return: if graph is None return jsonify({"error":"Graph not
found"}), 404; if select_graph raises an exception treat it as a DB error and
return a 500. Replace the current logging call with a safe log that uses %r for
the namespaced value and include exc_info=True (e.g., logging.error("Failed to
select graph %r", namespaced, exc_info=True)) so stack traces are preserved and
log forging is mitigated, and ensure no graph attributes are accessed before the
None check.
| const Graph = ForceGraph()(document.getElementById('schema-graph')) | ||
| .graphData(data) | ||
| .nodeId('name') | ||
| // Custom node renderer: draw table-like boxes with columns |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-guard schema container and set nodeId before graphData
- Guard against a missing #schema-graph element to prevent runtime errors.
- Set nodeId('name') before graphData for deterministic link resolution.
-export function showGraph(data) {
-
- const Graph = ForceGraph()(document.getElementById('schema-graph'))
- .graphData(data)
- .nodeId('name')
+export function showGraph(data) {
+ const el = document.getElementById('schema-graph');
+ if (!el) {
+ console.warn('showGraph: #schema-graph not found');
+ return;
+ }
+ const Graph = ForceGraph()(el)
+ .nodeId('name')
+ .graphData(data)📝 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 Graph = ForceGraph()(document.getElementById('schema-graph')) | |
| .graphData(data) | |
| .nodeId('name') | |
| // Custom node renderer: draw table-like boxes with columns | |
| export function showGraph(data) { | |
| const el = document.getElementById('schema-graph'); | |
| if (!el) { | |
| console.warn('showGraph: #schema-graph not found'); | |
| return; | |
| } | |
| const Graph = ForceGraph()(el) | |
| .nodeId('name') | |
| .graphData(data) | |
| // Custom node renderer: draw table-like boxes with columns |
🤖 Prompt for AI Agents
In api/static/js/modules/schema.js around lines 4 to 7, the code assumes
document.getElementById('schema-graph') exists and calls ForceGraph() directly
on it and calls nodeId after graphData; change to first query and null-guard the
container (if not found, return/do nothing) and then instantiate ForceGraph with
that container, call .nodeId('name') before .graphData(data) so node identifiers
are set prior to resolving links; ensure the function exits gracefully when the
element is missing.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/static/css/responsive.css (1)
15-18: Stale selector: update to new sidebar class or rule won’t fire on mobile.Mobile overlay rule still targets #menu-container.open, but the container was renamed to .sidebar-container. As-is, chat won’t reset its margin when the sidebar opens on small screens.
Apply:
- #menu-container.open~#chat-container { + .sidebar-container.open ~ #chat-container { margin-left: 0; }
🧹 Nitpick comments (2)
api/static/css/responsive.css (2)
22-28: Use dynamic viewport units for better mobile behavior.100vh has known issues on iOS/Android due to browser chrome. Prefer 100dvh for correct full-height overlay.
.sidebar-container { position: fixed; top: 0; left: 0; - height: 100vh; + height: 100dvh; z-index: 999; }
124-130: Repositioning GitHub button: verify overlap and consider responsive spacing.right: 60px may collide with the user-profile button on narrow screens or with the new left toolbar changes. Consider responsive spacing via clamp or, better, place both in a flex container to avoid absolute magic numbers.
Option A (minimal CSS-only tweak):
#github-link-btn { top: 15px; - right: 60px; + right: clamp(48px, 6vw, 80px); padding: 6px 8px; font-size: 12px; height: 40px; }Option B (preferred): group top-right actions in a flex container and drop absolute positioning for intrinsic layout (can follow up with a concrete snippet if you share the container markup).
📜 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 (7)
api/static/css/buttons.css(2 hunks)api/static/css/menu.css(3 hunks)api/static/css/responsive.css(2 hunks)api/static/js/app.js(4 hunks)api/static/js/modules/schema.js(1 hunks)api/static/js/modules/ui.js(2 hunks)api/templates/components/left_toolbar.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- api/static/js/modules/schema.js
- api/static/css/buttons.css
- api/templates/components/left_toolbar.j2
- api/static/css/menu.css
- api/static/js/app.js
- api/static/js/modules/ui.js
🧰 Additional context used
📓 Path-based instructions (1)
api/static/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Store frontend static assets under api/static/
Files:
api/static/css/responsive.css
⏰ 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
| /* Menu Responsive */ | ||
| @media (max-width: 768px) { | ||
| #menu-container { | ||
| .sidebar-container { | ||
| position: fixed; | ||
| top: 0; | ||
| left: 0; | ||
| height: 100vh; | ||
| z-index: 999; | ||
| } | ||
| #menu-container.open { | ||
|
|
||
| .sidebar-container.open { | ||
| width: 80vw; | ||
| padding: 15px; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Repo-wide cleanup after selector rename.
To prevent similar regressions, grep for leftover #menu-container, #side-menu-button, and #theme-toggle-btn occurrences across CSS/JS/templates and update/remove them.
🏁 Script executed:
#!/bin/bash
# Find stale selectors/ids after the toolbar/sidebar refactor
set -euo pipefail
echo "Searching for legacy selectors..."
rg -n -S -C2 --glob 'api/**' '#menu-container|#side-menu-button|#theme-toggle-btn'
echo "Check for sidebar open-state rules that may need mobile overrides..."
rg -n -S -C2 --glob 'api/**' '\.sidebar-container\.open'Length of output: 1955
Cleanup legacy selectors and ensure consistent sidebar styles
The refactor renamed menu and toolbar elements, but the following leftover selectors and duplicate styles remain and should be removed or updated:
-
api/static/js/modules/config.js
sideMenuButton: '#side-menu-button'menuContainer: '#menu-container'
-
api/static/css/responsive.css
- Mobile rule targeting
#menu-container.open ~ #chat-container(lines 15–17)
- Mobile rule targeting
-
api/static/css/buttons.css
#theme-toggle-btnselector (lines 139–141)
Additionally, there’s a duplicate .sidebar-container.open rule in api/static/css/menu.css (lines 19–21) with width: 30dvw; padding: 20px; that conflicts with the mobile override in responsive.css. Please remove or consolidate these styles so the sidebar’s open state is governed by a single, consistent media-query rule.
🤖 Prompt for AI Agents
api/static/css/responsive.css (around lines 20–34) plus related files:
api/static/js/modules/config.js (sideMenuButton/menuContainer),
api/static/css/responsive.css (mobile rule targeting #menu-container.open ~
#chat-container at ~lines 15–17), api/static/css/buttons.css (selector
#theme-toggle-btn at ~lines 139–141), and api/static/css/menu.css (duplicate
.sidebar-container.open at lines 19–21): remove the legacy selectors from
config.js (delete or update sideMenuButton and menuContainer entries), remove
the obsolete #menu-container.open ~ #chat-container mobile rule from
responsive.css, delete the stray #theme-toggle-btn selector from buttons.css,
and remove the duplicate .sidebar-container.open rule in menu.css; then
consolidate the sidebar open-state styling into one media-query rule (keep the
responsive.css mobile rule as the single source of truth—e.g., width: 80vw and
padding: 15px for .sidebar-container.open) so there is no conflicting duplicate
across files.
| .sidebar-container.open { | ||
| width: 80vw; | ||
| padding: 15px; | ||
| } | ||
| } |
There was a problem hiding this comment.
Explicitly override desktop open-state offset on mobile.
Cross-file context notes .sidebar-container.open has left: 48px in menu.css. Without overriding here, the drawer may be shifted on mobile. Set left: 0 in the mobile open state to ensure a true overlay.
.sidebar-container.open {
+ left: 0;
width: 80vw;
padding: 15px;
}📝 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.
| .sidebar-container.open { | |
| width: 80vw; | |
| padding: 15px; | |
| } | |
| } | |
| .sidebar-container.open { | |
| left: 0; | |
| width: 80vw; | |
| padding: 15px; | |
| } | |
| } |
🤖 Prompt for AI Agents
In api/static/css/responsive.css around lines 30-34, the mobile open-state for
.sidebar-container.open currently sets width and padding but doesn't override
the desktop left: 48px from menu.css; update this mobile rule to set left: 0 for
the .sidebar-container.open selector (and if menu.css uses higher specificity or
inline rules, add !important to the left: 0 declaration) so the drawer becomes a
true full-overlay on mobile.
2 participants
Summary by CodeRabbit
New Features
UI / Style
Accessibility
Chores