fix: strip demo graph prefix for display and fix schema table names#514
Open
galshubeli wants to merge 5 commits intostagingfrom
Open
fix: strip demo graph prefix for display and fix schema table names#514galshubeli wants to merge 5 commits intostagingfrom
galshubeli wants to merge 5 commits intostagingfrom
Conversation
The list graphs endpoint now returns structured objects with id, name, and is_demo fields so the frontend can display demo graphs without the GENERAL_PREFIX while still using the full id for API calls. Hardcoded demo graph name checks are replaced with the is_demo flag. Also fixes table names not rendering in the schema viewer by using schemaNode.name instead of the undefined node.displayName[1]. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Completed Working on "Code Review"✅ Review publishing completed successfully. Posted comments from all chunks and submitted final review. ✅ Workflow completed successfully. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request replaces string-/prefix-based demo detection with an explicit Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend (UI)
participant DBService as DatabaseService
participant Backend as Backend API
participant DB as FalkorDB
User->>Frontend: Request graphs / interact
Frontend->>DBService: getGraphs()
DBService->>Backend: GET /graphs
Backend->>DB: Query graphs
DB-->>Backend: return graph records
Backend->>Backend: build list of {id, name, is_demo}
Backend-->>DBService: [{id,name,is_demo}, ...]
DBService->>DBService: parse response (handle legacy strings)
DBService-->>Frontend: Graph[] with is_demo
Frontend->>Frontend: use selectedGraph.is_demo for UI & delete logic
Frontend->>User: render suggestions, badges, controls based on is_demo
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the QueryWeaver-pr-514 environment in queryweaver
|
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-514
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Found 2 MAJOR issues across 2 files.
Key themes
- Backward compatibility risk in demo-graph behavior after switching to
is_demo-only checks. - Type contract drift between frontend types and backend API guarantees (
is_demotreated as optional where behavior depends on it).
Recommended next steps
- Add a temporary fallback predicate for demo detection (e.g., id/name-based) to avoid regressions for legacy/persisted graph objects.
- Make
Graph.is_demorequired in frontend types and ensure list-graphs responses always include it. - Add/adjust tests for demo suggestion-card visibility and demo-protection behavior with both legacy and current graph payload shapes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the graphs listing contract to return structured graph objects (including a demo flag) so the UI can display demo graphs without the internal prefix while continuing to use full graph IDs for API operations. It also fixes schema table labels in the schema viewer by using the correct field.
Changes:
- Update
/graphsto return{ id, name, is_demo }objects instead of plain string IDs. - Update frontend demo-graph handling to rely on
is_demoinstead of hardcoded name/prefix checks. - Fix schema viewer table name rendering by using
schemaNode.name.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/types/api.ts | Extends Graph typing to include is_demo and makes timestamps optional to accommodate list response shape. |
| app/src/pages/Index.tsx | Uses is_demo for demo detection and removes special-casing of demo display names. |
| app/src/components/schema/SchemaViewer.tsx | Fixes displayed table name source for canvas labels. |
| app/src/components/chat/ChatInterface.tsx | Shows suggestion cards based on selectedGraph.is_demo rather than hardcoded demo IDs/names. |
| api/routes/graphs.py | Changes list-graphs endpoint to return structured items with id/name/is_demo and strips prefix for display name. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/pages/Index.tsx (1)
184-185: PassisDemointo the handler instead of re-queryinggraphs.At Line 184, recomputing with
graphs.find(...)is unnecessary and can drift from the clicked row state. Prefer passing the already-knownisDemofrom the row click site directly tohandleDeleteGraph.♻️ Suggested simplification
- const handleDeleteGraph = async (graphId: string, graphName: string, event: React.MouseEvent) => { + const handleDeleteGraph = async ( + graphId: string, + graphName: string, + isDemo: boolean, + event: React.MouseEvent + ) => { event.stopPropagation(); // Prevent dropdown from closing/selecting - const isDemo = graphs.find(g => g.id === graphId)?.is_demo || false; if (isRefreshingSchema) return; setDatabaseToDelete({ id: graphId, name: graphName, isDemo }); setShowDeleteModal(true); };// call site (outside this hunk) onClick={(e) => { if (isDemo || isRefreshingSchema || isChatProcessing) return; handleDeleteGraph(graph.id, graph.name, isDemo, e); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Index.tsx` around lines 184 - 185, The code recomputes isDemo via graphs.find(...) causing possible state drift; update handleDeleteGraph to accept an isDemo boolean parameter (e.g., handleDeleteGraph(graphId, graphName, isDemo, event)) and use that passed value instead of querying graphs inside handleDeleteGraph, then update the call site(s) (the onClick handler that currently checks isDemo/isRefreshingSchema/isChatProcessing) to pass the row's isDemo (graph.is_demo) through to handleDeleteGraph; ensure all references to the old signature are updated to the new signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/routes/graphs.py`:
- Around line 56-59: The current demo detection in graphs.py uses is_demo =
bool(GENERAL_PREFIX and graph_id.startswith(GENERAL_PREFIX)) and then truncates
display_name, which misclassifies user graphs whose visible name starts with
GENERAL_PREFIX; instead, use the demo flag that should be provided by the data
source (e.g., the list_databases response or DB record) rather than recomputing
from the stripped graph_id—lookup the originating field (e.g., the database
record's is_demo/isShared/isPublic flag returned by list_databases) and use that
to set is_demo and compute display_name, or if the data source cannot provide
it, enforce and validate at creation time that user graph names cannot begin
with GENERAL_PREFIX (reject/create-time validation) so the current startswith
logic remains safe; update occurrences of is_demo, graph_id and display_name
handling accordingly.
---
Nitpick comments:
In `@app/src/pages/Index.tsx`:
- Around line 184-185: The code recomputes isDemo via graphs.find(...) causing
possible state drift; update handleDeleteGraph to accept an isDemo boolean
parameter (e.g., handleDeleteGraph(graphId, graphName, isDemo, event)) and use
that passed value instead of querying graphs inside handleDeleteGraph, then
update the call site(s) (the onClick handler that currently checks
isDemo/isRefreshingSchema/isChatProcessing) to pass the row's isDemo
(graph.is_demo) through to handleDeleteGraph; ensure all references to the old
signature are updated to the new signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d8c34c4-a1a9-4617-8072-9b42d1e20479
📒 Files selected for processing (5)
api/routes/graphs.pyapp/src/components/chat/ChatInterface.tsxapp/src/components/schema/SchemaViewer.tsxapp/src/pages/Index.tsxapp/src/types/api.ts
Move is_demo classification into list_databases so it comes from the data source rather than re-inferring via startsWith in the route. This prevents user graphs whose name happens to start with GENERAL_PREFIX from being misclassified as demo graphs. Also makes is_demo required on the Graph type and passes isDemo directly to handleDeleteGraph. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-514
The getGraphs helper now extracts id strings from the object response
so existing E2E tests that call string methods like endsWith continue
to work with the new {id, name, is_demo} response format.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-514
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
api/core/schema_loader.py (1)
147-157: Tighten the return type forlist_databases.
list[dict]/list[dict] = []is too loose for this now-stable response shape (id,name,is_demo). Please use a typed structure (e.g.,TypedDict) so static checks catch contract drift.♻️ Proposed refactor
-from typing import AsyncGenerator, Optional +from typing import AsyncGenerator, Optional, TypedDict + +class GraphListItem(TypedDict): + id: str + name: str + is_demo: bool @@ async def list_databases( user_id: str, general_prefix: Optional[str] = None -) -> list[dict]: +) -> list[GraphListItem]: @@ - result: list[dict] = [] + result: list[GraphListItem] = []As per coding guidelines, "Use type hints throughout Python code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/core/schema_loader.py` around lines 147 - 157, The return type and local `result` list in `list_databases` are too permissive; define a TypedDict (e.g., `DatabaseInfo` or `GraphInfo` with keys `id: str`, `name: str`, `is_demo: bool`), import TypedDict from typing, change the function return annotation from `list[dict]` to `list[DatabaseInfo]`, and initialize `result: list[DatabaseInfo] = []`; update any places that construct dicts to produce `DatabaseInfo`-shaped dicts so static type checkers can validate the response shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/core/schema_loader.py`:
- Around line 151-153: The docstring says the returned dict key "id" should be
the full FalkorDB graph name for API calls, but the implementation at the
user-graph construction (lines around 163-170) sets "id" to the stripped display
name; change the runtime so the returned dict uses the original full graph name
for the "id" field while keeping the stripped value in "name" and preserving
"is_demo", i.e., when building each graph dict assign "id" from the
full/original graph name variable (not the stripped display name) so runtime
matches the docstring.
In `@app/src/services/database.ts`:
- Around line 41-53: getGraphs may return a non-array when payload is
unexpected; ensure the function always returns a Graph[] by validating
graphNames before returning. In getGraphs, after computing const graphNames =
data.graphs || data || [], validate that graphNames is an array of either
strings or objects: if Array.isArray(graphNames) && graphNames.length > 0 &&
typeof graphNames[0] === 'string' keep the existing string->Graph mapping, else
if Array.isArray(graphNames) && graphNames.every(item => item && typeof item ===
'object') cast/map each item to Graph (ensuring id/name/is_demo fields exist
with safe defaults), otherwise return an empty array; use the existing
graphNames identifier and the getGraphs function name to locate and update the
code.
In `@e2e/logic/api/apiCalls.ts`:
- Around line 160-165: getGraphs must always return an array shape; instead of
returning data as-is, coerce/normalize the parsed response into an array of ids.
After parsing response.json() in getGraphs, handle these cases: if Array -> map
GraphListItem to id; else if object with an "id" property -> return [data.id];
else if object with an "items" array -> map items to ids; otherwise return an
empty array. Update the branch around the GraphListItem check to normalize all
non-array payloads into a safe array return so GraphsListResponse contract is
preserved.
---
Nitpick comments:
In `@api/core/schema_loader.py`:
- Around line 147-157: The return type and local `result` list in
`list_databases` are too permissive; define a TypedDict (e.g., `DatabaseInfo` or
`GraphInfo` with keys `id: str`, `name: str`, `is_demo: bool`), import TypedDict
from typing, change the function return annotation from `list[dict]` to
`list[DatabaseInfo]`, and initialize `result: list[DatabaseInfo] = []`; update
any places that construct dicts to produce `DatabaseInfo`-shaped dicts so static
type checkers can validate the response shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1825fba6-ed92-49d3-9dc2-9216ad2af98e
📒 Files selected for processing (6)
api/core/schema_loader.pyapp/src/pages/Index.tsxapp/src/services/database.tsapp/src/types/api.tse2e/logic/api/apiCalls.tse2e/logic/api/apiResponses.ts
✅ Files skipped from review due to trivial changes (1)
- e2e/logic/api/apiResponses.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/pages/Index.tsx
Comment on lines
+151
to
+153
| Returns a list of dicts with keys ``id`` (the full FalkorDB graph name | ||
| used for API calls), ``name`` (the display name with namespace prefix | ||
| stripped) and ``is_demo`` (whether the graph is a shared demo graph). |
Contributor
There was a problem hiding this comment.
Docstring for id does not match runtime behavior.
Line 151-153 says id is the full FalkorDB graph name for API calls, but Line 163 sets user graph id to the stripped display name. This can mislead callers and future maintainers.
Also applies to: 163-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/core/schema_loader.py` around lines 151 - 153, The docstring says the
returned dict key "id" should be the full FalkorDB graph name for API calls, but
the implementation at the user-graph construction (lines around 163-170) sets
"id" to the stripped display name; change the runtime so the returned dict uses
the original full graph name for the "id" field while keeping the stripped value
in "name" and preserving "is_demo", i.e., when building each graph dict assign
"id" from the full/original graph name variable (not the stripped display name)
so runtime matches the docstring.
Comment on lines
+160
to
+165
| const data = await response.json(); | ||
| // Backend now returns objects with {id, name, is_demo}; extract ids for backward compat | ||
| if (Array.isArray(data) && data.length > 0 && typeof data[0] === 'object') { | ||
| return (data as GraphListItem[]).map((g) => g.id); | ||
| } | ||
| return data; |
Contributor
There was a problem hiding this comment.
Ensure getGraphs() always returns an array shape.
Line 165 returns data as-is; if payload shape drifts (object wrapper, error body, etc.), this breaks the GraphsListResponse contract and can destabilize tests expecting array methods.
💡 Safer shape-guard
- const data = await response.json();
- // Backend now returns objects with {id, name, is_demo}; extract ids for backward compat
- if (Array.isArray(data) && data.length > 0 && typeof data[0] === 'object') {
- return (data as GraphListItem[]).map((g) => g.id);
- }
- return data;
+ const data: unknown = await response.json();
+ if (!Array.isArray(data)) {
+ throw new Error('Unexpected /graphs response shape');
+ }
+ if (data.length === 0) return [];
+ if (typeof data[0] === 'string') return data as GraphsListResponse;
+ if (typeof data[0] === 'object' && data[0] !== null) {
+ return (data as GraphListItem[]).map((g) => g.id);
+ }
+ throw new Error('Unexpected /graphs item type');📝 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.
Suggested change
| const data = await response.json(); | |
| // Backend now returns objects with {id, name, is_demo}; extract ids for backward compat | |
| if (Array.isArray(data) && data.length > 0 && typeof data[0] === 'object') { | |
| return (data as GraphListItem[]).map((g) => g.id); | |
| } | |
| return data; | |
| const data: unknown = await response.json(); | |
| if (!Array.isArray(data)) { | |
| throw new Error('Unexpected /graphs response shape'); | |
| } | |
| if (data.length === 0) return []; | |
| if (typeof data[0] === 'string') return data as GraphsListResponse; | |
| if (typeof data[0] === 'object' && data[0] !== null) { | |
| return (data as GraphListItem[]).map((g) => g.id); | |
| } | |
| throw new Error('Unexpected /graphs item type'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/logic/api/apiCalls.ts` around lines 160 - 165, getGraphs must always
return an array shape; instead of returning data as-is, coerce/normalize the
parsed response into an array of ids. After parsing response.json() in
getGraphs, handle these cases: if Array -> map GraphListItem to id; else if
object with an "id" property -> return [data.id]; else if object with an "items"
array -> map items to ids; otherwise return an empty array. Update the branch
around the GraphListItem check to normalize all non-array payloads into a safe
array return so GraphsListResponse contract is preserved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The list graphs endpoint now returns structured objects with id, name, and is_demo fields so the frontend can display demo graphs without the GENERAL_PREFIX while still using the full id for API calls. Hardcoded demo graph name checks are replaced with the is_demo flag. Also fixes table names not rendering in the schema viewer by using schemaNode.name instead of the undefined node.displayName[1].
Summary by CodeRabbit