Conversation
…react-force-graph-2d - Added @falkordb/canvas dependency to package.json and package-lock.json. - Refactored SchemaViewer component to utilize FalkorDBCanvas for rendering schema data. - Updated schema data handling to remap node IDs and create a nodesMap for efficient access. - Removed all references and dependencies related to react-force-graph-2d. - Implemented dynamic loading of canvas and adjusted theme colors for better visualization. - Enhanced zoom and center functionalities to work with the new canvas implementation.
…ckage.json and package-lock.json
…e.json and package-lock.json
Bumps the pip group with 1 update in the / directory: [urllib3](https://github.com/urllib3/urllib3). Updates `urllib3` from 2.6.2 to 2.6.3 - [Release notes](https://github.com/urllib3/urllib3/releases) - [Changelog](https://github.com/urllib3/urllib3/blob/main/CHANGES.rst) - [Commits](urllib3/urllib3@2.6.2...2.6.3) --- updated-dependencies: - dependency-name: urllib3 dependency-version: 2.6.3 dependency-type: indirect dependency-group: pip ... Signed-off-by: dependabot[bot] <support@github.com>
Completed Working on "Code Review"✅ Workflow completed successfully. |
|
🚅 Deployed to the QueryWeaver-pr-372 environment in queryweaver
|
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-372
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Dependabot Actions config, sets HSTS header in SecurityMiddleware with tests, introduces Redis-backed MEMORY_TTL for memory graphs, refactors SchemaViewer to use Changes
Sequence Diagram(s)sequenceDiagram
participant App as MemoryTool
participant Falkor as FalkorDriver
participant Redis as Redis
App->>Falkor: create_vector_index(memory_db_name)
Falkor-->>App: index_created
alt MEMORY_TTL_SECONDS set
App->>Redis: EXPIRE(memory_key, MEMORY_TTL_SECONDS)
Redis-->>App: OK / Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency ReviewThe following issues were found:
|
Bump urllib3 from 2.6.2 to 2.6.3 in the pip group across 1 directory
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-372
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings by severity
- BLOCKER: 0
- CRITICAL: 0
- MAJOR: 1
- MINOR: 0
- SUGGESTION: 0
- PRAISE: 0
Affected file: .github/dependabot.yml
Key theme
- Dependabot workflow alignment – the new
github-actionsentry currently raises PRs againstmain, breaking the intended staging-first gate used by other ecosystems.
Next steps
- Specify
target-branch: "staging"under thegithub-actionsupdate entry so automation consistently targets the staging branch before hittingmain.
No blocking or critical issues were identified, so this PR can proceed once the above adjustment is made.
…e.json and package-lock.json
….json and package-lock.json
…d package-lock.json
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-372
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/app_factory.py (1)
36-53: Apply HSTS to early-return responses too.The 403 responses returned before
call_nextnever receive the HSTS header, so it’s not truly applied to all responses.🐛 Proposed fix
async def dispatch(self, request: Request, call_next): + hsts_value = "max-age=31536000; includeSubDomains; preload" + + def _apply_hsts(resp): + resp.headers["Strict-Transport-Security"] = hsts_value + return resp + # Block directory access in static files if request.url.path.startswith(self.STATIC_PREFIX): # Remove /static/ prefix to get the actual path filename = request.url.path[len(self.STATIC_PREFIX) :] # Basic security check for directory traversal if not filename or "../" in filename or filename.endswith("/"): - return JSONResponse(status_code=403, content={"detail": "Forbidden"}) + return _apply_hsts( + JSONResponse(status_code=403, content={"detail": "Forbidden"}) + ) response = await call_next(request) - - # Add HSTS header to prevent man-in-the-middle attacks - # max-age=31536000: 1 year in seconds - # includeSubDomains: apply to all subdomains - # preload: eligible for browser HSTS preload lists - hsts_value = "max-age=31536000; includeSubDomains; preload" - response.headers["Strict-Transport-Security"] = hsts_value - - return response + return _apply_hsts(response)
🧹 Nitpick comments (2)
tests/test_hsts_header.py (1)
12-15: CloseTestClientto avoid resource leaks.Using a context manager in the fixture ensures the client is properly closed after each test.
♻️ Proposed update
`@pytest.fixture` def client(self): """Create a test client.""" - return TestClient(app) + with TestClient(app) as client: + yield clientapi/app_factory.py (1)
47-52: Consider setting HSTS only on HTTPS responses.Best practice is to emit HSTS only when the request is HTTPS (or forwarded as HTTPS).
♻️ Possible tweak
- response.headers["Strict-Transport-Security"] = hsts_value + if request.url.scheme == "https": + response.headers["Strict-Transport-Security"] = hsts_value
Bumps [jsonschema](https://github.com/python-jsonschema/jsonschema) from 4.25.1 to 4.26.0. - [Release notes](https://github.com/python-jsonschema/jsonschema/releases) - [Changelog](https://github.com/python-jsonschema/jsonschema/blob/main/CHANGELOG.rst) - [Commits](python-jsonschema/jsonschema@v4.25.1...v4.26.0) --- updated-dependencies: - dependency-name: jsonschema dependency-version: 4.26.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
feat: integrate @falkordb/canvas for schema visualization and remove …
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-372
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/package.json`:
- Line 14: Remove the unnecessary direct dependency on "preact" from
package.json since it's only pulled in transitively by float-tooltip; delete the
"preact" entry and run an install to ensure no breakage. Also verify the
"@falkordb/canvas" dependency version: confirm that using
"@falkordb/canvas@0.0.29" is intentional (or bump to a stable release) by
checking changelogs/maintainer notes and run the app/tests to validate stability
before finalizing the change.
In `@app/src/components/schema/SchemaViewer.tsx`:
- Around line 189-197: The handleCenter callback contains a stray debug
console.log("Stop"); remove that console.log from the handleCenter function (in
the same block that references canvasRef.current and calls canvas.zoomToFit())
so the callback only retrieves the canvas and calls canvas.zoomToFit() when
present.
- Around line 289-293: The code directly accesses node.displayName[1] in the
ctx.fillText call which can throw if displayName is undefined or has fewer than
2 elements; update the rendering logic in SchemaViewer (the block around
ctx.fillText) to safely compute a text value first (e.g., const secondary =
Array.isArray(node.displayName) && node.displayName.length > 1 ?
node.displayName[1] : '') and then pass that safe secondary value to
ctx.fillText so missing or short displayName arrays don't cause runtime errors.
- Around line 150-155: The link-mapping fallback returns string IDs but
SchemaLink expects numbers; update the data.links mapping to coerce the resolved
IDs to numbers (use Number(...) or parseInt(..., 10)) for both source and target
when reading from oldIdToNewId or falling back to link.source/link.target, and
ensure this conversion is applied in the map callback that creates the new link
objects so source and target are always numbers (refer to data.links,
oldIdToNewId, and SchemaLink).
🧹 Nitpick comments (1)
app/src/components/schema/SchemaViewer.tsx (1)
85-90: Add error handling for the dynamic import.If the dynamic import fails (e.g., network issues, bundle errors), the promise rejection is unhandled and
canvasLoadedwill remainfalsewith no user feedback.🛡️ Proposed fix
// Load falkordb-canvas dynamically useEffect(() => { - import('@falkordb/canvas').then(() => { - setCanvasLoaded(true); - }); + import('@falkordb/canvas') + .then(() => { + setCanvasLoaded(true); + }) + .catch((error) => { + console.error('Failed to load canvas:', error); + toast({ + title: 'Failed to Load Canvas', + description: 'Could not load the schema visualization component', + variant: 'destructive', + }); + }); }, []);
| const handleCenter = useCallback(() => { | ||
| console.log("Stop"); | ||
|
|
||
| const canvas = canvasRef.current; | ||
|
|
||
| if (canvas) { | ||
| canvas.zoomToFit(); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Remove debug console.log.
Line 190 contains a debug statement console.log("Stop") that should be removed before merging.
🧹 Proposed fix
const handleCenter = useCallback(() => {
- console.log("Stop");
-
const canvas = canvasRef.current;
if (canvas) {
canvas.zoomToFit();
}
}, []);📝 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 handleCenter = useCallback(() => { | |
| console.log("Stop"); | |
| const canvas = canvasRef.current; | |
| if (canvas) { | |
| canvas.zoomToFit(); | |
| } | |
| }, []); | |
| const handleCenter = useCallback(() => { | |
| const canvas = canvasRef.current; | |
| if (canvas) { | |
| canvas.zoomToFit(); | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
In `@app/src/components/schema/SchemaViewer.tsx` around lines 189 - 197, The
handleCenter callback contains a stray debug console.log("Stop"); remove that
console.log from the handleCenter function (in the same block that references
canvasRef.current and calls canvas.zoomToFit()) so the callback only retrieves
the canvas and calls canvas.zoomToFit() when present.
| ctx.fillText( | ||
| node.displayName[1], | ||
| node.x || 0, | ||
| (node.y || 0) - nodeHeight / 2 + headerHeight / 2 + padding / 2 | ||
| ); |
There was a problem hiding this comment.
Potential runtime error accessing displayName[1].
node.displayName[1] is accessed without validating that displayName exists and has at least two elements. If the canvas library provides nodes with missing or incomplete displayName, this will throw a runtime error.
🛡️ Proposed fix
ctx.fillText(
- node.displayName[1],
+ node.displayName?.[1] ?? node.data?.name ?? '',
node.x || 0,
(node.y || 0) - nodeHeight / 2 + headerHeight / 2 + padding / 2
);📝 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.
| ctx.fillText( | |
| node.displayName[1], | |
| node.x || 0, | |
| (node.y || 0) - nodeHeight / 2 + headerHeight / 2 + padding / 2 | |
| ); | |
| ctx.fillText( | |
| node.displayName?.[1] ?? node.data?.name ?? '', | |
| node.x || 0, | |
| (node.y || 0) - nodeHeight / 2 + headerHeight / 2 + padding / 2 | |
| ); |
🤖 Prompt for AI Agents
In `@app/src/components/schema/SchemaViewer.tsx` around lines 289 - 293, The code
directly accesses node.displayName[1] in the ctx.fillText call which can throw
if displayName is undefined or has fewer than 2 elements; update the rendering
logic in SchemaViewer (the block around ctx.fillText) to safely compute a text
value first (e.g., const secondary = Array.isArray(node.displayName) &&
node.displayName.length > 1 ? node.displayName[1] : '') and then pass that safe
secondary value to ctx.fillText so missing or short displayName arrays don't
cause runtime errors.
…ma-4.26.0 Bump jsonschema from 4.25.1 to 4.26.0
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-372
Add ttl to Memory graphs
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-372
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@api/memory/graphiti_tool.py`:
- Around line 73-78: The README claim that TTL is refreshed on every user
interaction is incorrect because _refresh_ttl is only called in create(); update
per-interaction methods (at least search_memories, add_new_memory,
save_query_memory and any other public handlers) to call await
self._refresh_ttl() after successful DB operations so the Redis EXPIRE for
self.memory_db_name using self.MEMORY_TTL_SECONDS is renewed, or alternatively
update the README if you prefer not to change runtime behavior.
In `@Dockerfile`:
- Around line 43-50: The RUN chain currently uses "apt-get update && apt-get
remove -y nodejs || true" which allows failures of apt-get update to be
swallowed; change that to ensure only the remove is optional by grouping the
remove with || true (e.g., "apt-get update && (apt-get remove -y nodejs ||
true)") or separate into two RUN steps so apt-get update failure fails the
build; also add --no-install-recommends to the "apt-get install -y nodejs"
invocation to avoid pulling unnecessary packages and keep the rest of the
sequence (curl setup_22.x, second apt-get update, rm -rf /var/lib/apt/lists/*,
and the final node --version && npm --version) intact.
🧹 Nitpick comments (1)
api/memory/graphiti_tool.py (1)
52-58: Class-level env parsing is fail-fast but could produce a confusing error on invalid input.If
MEMORY_TTL_SECONDSis set to a non-numeric string,int()raises aValueErrorat import time with no indication which env var is at fault. A minor robustness improvement:Suggested improvement
- MEMORY_TTL_SECONDS: Optional[int] = ( - int(os.environ["MEMORY_TTL_SECONDS"]) - if os.environ.get("MEMORY_TTL_SECONDS") - else None - ) + _raw_ttl = os.environ.get("MEMORY_TTL_SECONDS") + MEMORY_TTL_SECONDS: Optional[int] = int(_raw_ttl) if _raw_ttl else NoneThis is fine as-is for a fail-fast approach; just noting the UX for operators.
| async def _refresh_ttl(self) -> None: | ||
| """Set a TTL on the memory graph key using Redis EXPIRE.""" | ||
| try: | ||
| await db.execute_command("EXPIRE", self.memory_db_name, self.MEMORY_TTL_SECONDS) | ||
| except RedisError as e: | ||
| logging.warning("Failed to refresh TTL for %s: %s", self.memory_db_name, e) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all calls to _refresh_ttl
rg -n --type=py '_refresh_ttl' .Repository: FalkorDB/QueryWeaver
Length of output: 209
🏁 Script executed:
# Find all calls to create() on MemoryTool
rg -n --type=py 'create\(' . | grep -i memoryRepository: FalkorDB/QueryWeaver
Length of output: 406
🏁 Script executed:
# Check the graphiti_tool.py file to see the full implementation
cat -n api/memory/graphiti_tool.py | head -150Repository: FalkorDB/QueryWeaver
Length of output: 6889
🏁 Script executed:
# Find the README and check what it says about TTL
find . -name "README*" -type f | head -5Repository: FalkorDB/QueryWeaver
Length of output: 114
🏁 Script executed:
# Look for how MemoryTool is used - search for imports and instantiation
rg -n --type=py 'from.*graphiti_tool|import.*MemoryTool' .Repository: FalkorDB/QueryWeaver
Length of output: 202
🏁 Script executed:
# Check text2sql.py around the create() calls to understand the pattern
sed -n '235,250p' api/core/text2sql.pyRepository: FalkorDB/QueryWeaver
Length of output: 697
🏁 Script executed:
# Check text2sql.py around line 725
sed -n '720,735p' api/core/text2sql.pyRepository: FalkorDB/QueryWeaver
Length of output: 761
🏁 Script executed:
# List all public methods in MemoryTool
rg -n --type=py '^\s+async def|^\s+def' api/memory/graphiti_tool.pyRepository: FalkorDB/QueryWeaver
Length of output: 1395
🏁 Script executed:
# Check README for the TTL statement
rg -n --type=md 'TTL|refresh' README.md | head -20Repository: FalkorDB/QueryWeaver
Length of output: 427
🏁 Script executed:
# Get more context from the full MemoryTool implementation
sed -n '140,300p' api/memory/graphiti_tool.pyRepository: FalkorDB/QueryWeaver
Length of output: 7796
TTL is only refreshed during create(), not on every user interaction.
The README (line 69) states "The TTL is refreshed on every user interaction, so active users keep their memory." However, _refresh_ttl is only called inside create(). The per-interaction methods (search_memories, add_new_memory, save_query_memory, etc.) operate on an existing MemoryTool instance and do not refresh the TTL. This means active users with ongoing interactions can still lose their memory graphs if create() isn't invoked on every request.
Call _refresh_ttl in the per-interaction methods (at least the most common entry points like search_memories and add_new_memory), or update the README to reflect the actual behavior.
🤖 Prompt for AI Agents
In `@api/memory/graphiti_tool.py` around lines 73 - 78, The README claim that TTL
is refreshed on every user interaction is incorrect because _refresh_ttl is only
called in create(); update per-interaction methods (at least search_memories,
add_new_memory, save_query_memory and any other public handlers) to call await
self._refresh_ttl() after successful DB operations so the Redis EXPIRE for
self.memory_db_name using self.MEMORY_TTL_SECONDS is renewed, or alternatively
update the README if you prefer not to change runtime behavior.
| RUN apt-get update \ | ||
| && apt-get remove -y nodejs || true \ | ||
| && rm -rf /var/lib/apt/lists/* \ | ||
| && curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \ | ||
| && apt-get update \ | ||
| && apt-get install -y nodejs \ | ||
| && rm -rf /var/lib/apt/lists/* \ | ||
| && node --version && npm --version |
There was a problem hiding this comment.
Operator precedence bug: || true swallows apt-get update failures too.
In shell, && and || are left-associative with equal precedence, so apt-get update && apt-get remove -y nodejs || true is parsed as (apt-get update && apt-get remove -y nodejs) || true. If apt-get update fails, the error is silently ignored.
Also, apt-get install -y nodejs on line 48 is missing --no-install-recommends, which inflates the image with unnecessary packages.
Proposed fix
-RUN apt-get update \
- && apt-get remove -y nodejs || true \
- && rm -rf /var/lib/apt/lists/* \
- && curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \
- && apt-get update \
- && apt-get install -y nodejs \
- && rm -rf /var/lib/apt/lists/* \
+RUN apt-get update \
+ && (apt-get remove -y nodejs || true) \
+ && rm -rf /var/lib/apt/lists/* \
+ && curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \
+ && apt-get update \
+ && apt-get install -y --no-install-recommends nodejs \
+ && rm -rf /var/lib/apt/lists/* \
&& node --version && npm --version📝 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.
| RUN apt-get update \ | |
| && apt-get remove -y nodejs || true \ | |
| && rm -rf /var/lib/apt/lists/* \ | |
| && curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \ | |
| && apt-get update \ | |
| && apt-get install -y nodejs \ | |
| && rm -rf /var/lib/apt/lists/* \ | |
| && node --version && npm --version | |
| RUN apt-get update \ | |
| && (apt-get remove -y nodejs || true) \ | |
| && rm -rf /var/lib/apt/lists/* \ | |
| && curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \ | |
| && apt-get update \ | |
| && apt-get install -y --no-install-recommends nodejs \ | |
| && rm -rf /var/lib/apt/lists/* \ | |
| && node --version && npm --version |
🧰 Tools
🪛 Trivy (0.69.1)
[error] 43-50: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get remove -y nodejs || true && rm -rf /var/lib/apt/lists/* && curl -fsSL https://deb.nodesource.com/setup_22.x | bash - && apt-get update && apt-get install -y nodejs && rm -rf /var/lib/apt/lists/* && node --version && npm --version'
Rule: DS-0029
(IaC/Dockerfile)
🤖 Prompt for AI Agents
In `@Dockerfile` around lines 43 - 50, The RUN chain currently uses "apt-get
update && apt-get remove -y nodejs || true" which allows failures of apt-get
update to be swallowed; change that to ensure only the remove is optional by
grouping the remove with || true (e.g., "apt-get update && (apt-get remove -y
nodejs || true)") or separate into two RUN steps so apt-get update failure fails
the build; also add --no-install-recommends to the "apt-get install -y nodejs"
invocation to avoid pulling unnecessary packages and keep the rest of the
sequence (curl setup_22.x, second apt-get update, rm -rf /var/lib/apt/lists/*,
and the final node --version && npm --version) intact.
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-391
Summary by CodeRabbit