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>
Bump urllib3 from 2.6.2 to 2.6.3 in the pip group across 1 directory
…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>
Add HSTS header to prevent MITM attacks
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 …
…ma-4.26.0 Bump jsonschema from 4.25.1 to 4.26.0
Add ttl to Memory graphs
Completed Working on "Code Review"✅ Workflow completed successfully. |
|
🚅 Deployed to the QueryWeaver-pr-391 environment in queryweaver
|
📝 WalkthroughWalkthroughIntroduces optional per-user Memory TTL (env/config + Redis EXPIRE), adds HSTS header to API responses, migrates frontend graph from ForceGraph2D to FalkorDBCanvas (with Preact), updates dependencies and Docker/node installation, and adds tests for the HSTS header and minor repo metadata changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as FastAPI Middleware
participant MemoryTool
participant Redis as Redis Server
Client->>API: request (creates/uses memory graph)
API->>MemoryTool: invoke create()/update graph
MemoryTool->>Redis: ensure index / persist graph
MemoryTool->>Redis: EXPIRE <memory_key> (MEMORY_TTL_SECONDS)
Redis-->>MemoryTool: OK
MemoryTool-->>API: return result
API-->>Client: response (includes Strict-Transport-Security header)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 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:
|
There was a problem hiding this comment.
Pull request overview
This pull request merges changes from the staging branch to main, introducing several security enhancements, frontend improvements, and new features.
Changes:
- Added HSTS security header to all responses to prevent man-in-the-middle attacks
- Replaced react-force-graph-2d with @falkordb/canvas for schema visualization with improved rendering
- Added optional TTL (time-to-live) feature for user memory graphs to enable automatic cleanup of idle data
- Updated dependencies (jsonschema 4.26.0, urllib3 2.6.3) and improved Dockerfile Node.js installation
- Added Dependabot configuration for GitHub Actions and improved spellcheck wordlist
Reviewed changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_hsts_header.py | New test file verifying HSTS header presence and configuration on all endpoints |
| app/src/components/schema/SchemaViewer.tsx | Major refactor replacing react-force-graph-2d with @falkordb/canvas library for schema visualization |
| app/package.json | Added @falkordb/canvas and preact dependencies, removed react-force-graph-2d |
| api/memory/graphiti_tool.py | Added TTL support for memory graphs with optional auto-expiry via MEMORY_TTL_SECONDS env var |
| api/app_factory.py | Added HSTS header to SecurityMiddleware for all responses |
| README.md | Documented new MEMORY_TTL_SECONDS feature and usage |
| .env.example | Added MEMORY_TTL_SECONDS configuration example |
| Pipfile & Pipfile.lock | Updated jsonschema to 4.26.0 and urllib3 to 2.6.3 |
| Dockerfile | Improved Node.js installation by removing conflicting packages first |
| .github/dependabot.yml | Added GitHub Actions ecosystem monitoring |
| .github/wordlist.txt | Added "TTL" to spellcheck whitelist |
| className="h-8 w-8 p-0 bg-card border-border text-muted-foreground hover:bg-foreground" | ||
| title="Zoom In" | ||
| > | ||
| <ZoomIn className="h-4 w-4" /> | ||
| </Button> | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={handleZoomOut} | ||
| className="h-8 w-8 p-0 bg-card border-border text-muted-foreground hover:bg-foreground" | ||
| title="Zoom Out" | ||
| > | ||
| <ZoomOut className="h-4 w-4" /> | ||
| </Button> | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={handleCenter} | ||
| className="h-8 w-8 p-0 bg-card border-border text-muted-foreground hover:bg-foreground" |
There was a problem hiding this comment.
The hover class changed from hover:bg-muted to hover:bg-foreground, which sets the background to the foreground text color on hover. This will likely result in poor contrast and unreadable button states. Consider using hover:bg-muted or hover:bg-accent instead for proper UI feedback.
| console.log("Stop"); | ||
|
|
There was a problem hiding this comment.
Debug console.log statement left in production code. This should be removed as it appears to be a debugging artifact with an unclear message ("Stop").
| console.log("Stop"); |
| ctx.textAlign = 'center'; | ||
| ctx.textBaseline = 'middle'; | ||
| ctx.fillText( | ||
| node.displayName[1], |
There was a problem hiding this comment.
Accessing node.displayName[1] without verifying that displayName exists on the GraphNode type. According to the convertToCanvasData function, nodes are created with a data.name property but not a displayName property. This will likely cause a runtime error. Consider using schemaNode.name instead, which is the table name from the schema data.
| node.displayName[1], | |
| schemaNode.name, |
| </div> | ||
| )} | ||
| {!loading && canvasLoaded && schemaData && schemaData.nodes.length > 0 && ( | ||
| <falkordb-canvas ref={canvasRef} node-mode='replace' /> |
There was a problem hiding this comment.
Using a custom element <falkordb-canvas> without a TypeScript declaration will cause type errors during compilation. You need to add a TypeScript declaration file (e.g., app/src/types/falkordb-canvas.d.ts) to extend the JSX namespace with:
declare namespace JSX {
interface IntrinsicElements {
'falkordb-canvas': React.DetailedHTMLProps<React.HTMLAttributes<HTMLElement>, HTMLElement> & {
ref?: React.Ref<FalkorDBCanvas>;
'node-mode'?: string;
};
}
}| if cls.MEMORY_TTL_SECONDS is not None: | ||
| await self._refresh_ttl() |
There was a problem hiding this comment.
The TTL is only refreshed during object creation, but according to the README documentation added in this PR, "The TTL is refreshed on every user interaction, so active users keep their memory." Consider calling _refresh_ttl() at the end of user interaction methods like add_new_memory, save_query_memory, retrieve_similar_queries, and search_memories to prevent active users' memory from expiring.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @.github/dependabot.yml:
- Around line 8-11: The Dependabot entry for package-ecosystem "github-actions"
is missing target-branch and will open PRs against the default branch; update
the "github-actions" block (the package-ecosystem: "github-actions" entry) to
include target-branch: "staging" so its PRs route to the staging branch like the
pip and npm entries.
In `@api/memory/graphiti_tool.py`:
- Around line 54-58: The current top-level constant MEMORY_TTL_SECONDS will
raise ValueError during import if the environment variable is non-numeric;
change the initialization of MEMORY_TTL_SECONDS to defensively parse the env var
(use os.environ.get) and wrap int(...) in a try/except (or validate with
str.isdigit()/regex) so non-integer values result in a safe fallback (e.g.,
None) and optionally log or warn; update the symbol MEMORY_TTL_SECONDS
initialization accordingly so import-time crashes are prevented.
In `@app/package.json`:
- Line 14: Remove the unused "preact" dependency from package.json: delete the
"preact" entry so it no longer contributes to install/bundle size; keep
"@falkordb/canvas": "^0.0.29" as-is but be aware it's pre-1.0 (no code changes
to `@falkordb/canvas` required). Ensure package.json still validates (commas, JSON
structure) and run npm/yarn install and a quick project build to confirm nothing
breaks after removing "preact".
In `@app/src/components/schema/SchemaViewer.tsx`:
- Around line 189-191: Remove the debug console.log("Stop") from the
handleCenter callback in SchemaViewer (the useCallback named handleCenter) so it
no longer emits console output; simply delete the console.log line and keep the
existing logic of the callback intact.
- Around line 150-155: The current remapping in data.links uses the || fallback
which can return the original string ID and cause a type mismatch for
SchemaLink.source/target; change the mapping to explicitly resolve newSource =
oldIdToNewId.get(link.source) and newTarget = oldIdToNewId.get(link.target) and
if either is undefined either (a) throw or log and remove the link so the
missing node is surfaced, or (b) raise a TypeError with details (include
link.id, link.source, link.target) so callers know about the orphaned link;
update the code that uses data.links (the mapping block referencing oldIdToNewId
and SchemaLink.source/target) to perform this explicit check instead of using ||
fallback.
- Around line 86-90: The dynamic import in the useEffect that loads
'@falkordb/canvas' lacks error handling, so if import('@falkordb/canvas')
rejects the component silently stays in the "not loaded" state; update the
effect around import('@falkordb/canvas') (the useEffect block that calls
setCanvasLoaded) to add a .catch handler (or try/catch if using async) that logs
the error and updates component state (e.g., setCanvasLoaded(false) plus a new
setCanvasLoadError or similar boolean/string) so the UI can show an error
message or fallback UI when loading fails; ensure you continue to
setCanvasLoaded(true) on success and reference useEffect,
import('@falkordb/canvas'), setCanvasLoaded, and the new error state so
reviewers can locate the change.
- Around line 451-453: Add a side-effect import to ensure the global JSX
augmentation for the custom element is loaded: at the top of SchemaViewer.tsx
add import '@falkordb/canvas'; (no `type`) so TypeScript includes the module at
runtime and the JSX intrinsic element falkordb-canvas is recognized when
rendering the falkordb-canvas element in the SchemaViewer component.
In `@README.md`:
- Line 69: The README overstates TTL behavior — _refresh_ttl() is only invoked
from MemoryTool.create(), so update the sentence "The TTL is refreshed on every
user interaction" to accurately reflect that TTL is refreshed only when
MemoryTool.create() is called (or change the implementation to call
_refresh_ttl() on every interaction if you intend the original claim); reference
the _refresh_ttl() method and MemoryTool.create() in the updated wording so
readers know the exact trigger for TTL refresh.
🧹 Nitpick comments (3)
Dockerfile (1)
43-50: Add--no-install-recommendsto the Node.js install step.Line 48 installs
nodejswithout--no-install-recommends, which can pull in unnecessary packages and increase image size. This is consistent with how line 18 already uses the flag.Proposed fix
- && apt-get install -y nodejs \ + && apt-get install -y --no-install-recommends nodejs \tests/test_hsts_header.py (1)
17-43: Consider@pytest.mark.parametrizeto reduce duplication.Both test methods assert the same HSTS directives on different endpoints. A parameterized test would eliminate the repetition:
Proposed refactor
- def test_hsts_header_present(self, client): - """Test that the HSTS header is present in responses.""" - # Make a request to the root endpoint - response = client.get("/") - - # Verify HSTS header is present - assert "strict-transport-security" in response.headers - - # Verify header value contains required directives - hsts_header = response.headers["strict-transport-security"] - assert "max-age=31536000" in hsts_header - assert "includeSubDomains" in hsts_header - assert "preload" in hsts_header - - def test_hsts_header_on_api_endpoints(self, client): - """Test that the HSTS header is present on API endpoints.""" - # Test on graphs endpoint - response = client.get("/graphs") - - # Verify HSTS header is present - assert "strict-transport-security" in response.headers - - # Verify header value contains required directives - hsts_header = response.headers["strict-transport-security"] - assert "max-age=31536000" in hsts_header - assert "includeSubDomains" in hsts_header - assert "preload" in hsts_header + `@pytest.mark.parametrize`("path", ["/", "/graphs"]) + def test_hsts_header_present(self, client, path): + """Test that the HSTS header is present on all endpoints.""" + response = client.get(path) + + assert "strict-transport-security" in response.headers + hsts_header = response.headers["strict-transport-security"] + assert "max-age=31536000" in hsts_header + assert "includeSubDomains" in hsts_header + assert "preload" in hsts_headerapp/src/components/schema/SchemaViewer.tsx (1)
200-238:convertToCanvasDatais recreated on every theme change but is also a dependency of the canvas setup effect — this triggers a full data reload on theme toggle.Since
convertToCanvasDatadepends ontheme(viauseCallback(..., [theme])), every theme change causes the function identity to change, which triggers the canvas setupuseEffect(line 373), callingcanvas.setData(canvasData)again. This may cause unnecessary re-layouts of the graph (losing the user's pan/zoom position). Consider separating the color/background update from the data set, or only updating colors on theme change without resetting data.
| - package-ecosystem: "github-actions" | ||
| directory: "/" | ||
| schedule: | ||
| interval: "weekly" |
There was a problem hiding this comment.
Missing target-branch: "staging" — intentional?
The pip and npm entries both target staging, but this new github-actions entry omits target-branch, so Dependabot will open PRs against the default branch (main). If the intent is to keep all dependency PRs flowing through staging first, add target-branch: "staging" here as well.
Proposed fix
- package-ecosystem: "github-actions"
directory: "/"
+ target-branch: "staging"
schedule:
interval: "weekly"📝 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.
| - package-ecosystem: "github-actions" | |
| directory: "/" | |
| schedule: | |
| interval: "weekly" | |
| - package-ecosystem: "github-actions" | |
| directory: "/" | |
| target-branch: "staging" | |
| schedule: | |
| interval: "weekly" |
🤖 Prompt for AI Agents
In @.github/dependabot.yml around lines 8 - 11, The Dependabot entry for
package-ecosystem "github-actions" is missing target-branch and will open PRs
against the default branch; update the "github-actions" block (the
package-ecosystem: "github-actions" entry) to include target-branch: "staging"
so its PRs route to the staging branch like the pip and npm entries.
| MEMORY_TTL_SECONDS: Optional[int] = ( | ||
| int(os.environ["MEMORY_TTL_SECONDS"]) | ||
| if os.environ.get("MEMORY_TTL_SECONDS") | ||
| else None | ||
| ) |
There was a problem hiding this comment.
MEMORY_TTL_SECONDS will raise ValueError at import time if the env var is set to a non-integer string.
If someone sets MEMORY_TTL_SECONDS=1w or any non-numeric value, int(os.environ["MEMORY_TTL_SECONDS"]) will crash with a ValueError during module import, taking down the entire application. Consider wrapping in a try/except or validating gracefully.
🛡️ Proposed defensive parsing
- MEMORY_TTL_SECONDS: Optional[int] = (
- int(os.environ["MEMORY_TTL_SECONDS"])
- if os.environ.get("MEMORY_TTL_SECONDS")
- else None
- )
+ `@staticmethod`
+ def _parse_ttl() -> Optional[int]:
+ raw = os.environ.get("MEMORY_TTL_SECONDS")
+ if not raw:
+ return None
+ try:
+ return int(raw)
+ except ValueError:
+ logging.warning("Invalid MEMORY_TTL_SECONDS=%r, ignoring", raw)
+ return None
+
+ MEMORY_TTL_SECONDS: Optional[int] = _parse_ttl()🤖 Prompt for AI Agents
In `@api/memory/graphiti_tool.py` around lines 54 - 58, The current top-level
constant MEMORY_TTL_SECONDS will raise ValueError during import if the
environment variable is non-numeric; change the initialization of
MEMORY_TTL_SECONDS to defensively parse the env var (use os.environ.get) and
wrap int(...) in a try/except (or validate with str.isdigit()/regex) so
non-integer values result in a safe fallback (e.g., None) and optionally log or
warn; update the symbol MEMORY_TTL_SECONDS initialization accordingly so
import-time crashes are prevented.
| "preview": "vite preview" | ||
| }, | ||
| "dependencies": { | ||
| "@falkordb/canvas": "^0.0.29", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the current state of app/package.json to see if preact is present
cd app
echo "=== Checking app/package.json for preact and react dependencies ==="
grep -E '"(preact|react|@falkordb/canvas)"' package.json | head -20
echo -e "\n=== Full package.json snippet around line 14 ==="
sed -n '10,20p' package.json
echo -e "\n=== Full package.json snippet around line 50-60 ==="
sed -n '50,60p' package.jsonRepository: FalkorDB/QueryWeaver
Length of output: 1039
🏁 Script executed:
#!/bin/bash
# Check if `@falkordb/canvas` declares preact as a peer/regular dependency
# Search for any reference to `@falkordb/canvas` in the repo
echo "=== Searching for `@falkordb/canvas` references in the codebase ==="
rg "@falkordb/canvas" --type js --type jsx --type ts --type tsx -B 2 -A 2 | head -40
echo -e "\n=== Checking if there's a lockfile that shows `@falkordb/canvas` dependencies ==="
if [ -f app/package-lock.json ]; then
jq '.packages | to_entries[] | select(.key | contains("@falkordb/canvas")) | {key: .key, dependencies: .value.dependencies, peerDependencies: .value.peerDependencies}' app/package-lock.json 2>/dev/null | head -50
elif [ -f app/yarn.lock ]; then
echo "yarn.lock found, checking for `@falkordb/canvas`..."
grep -A 20 '@falkordb/canvas' app/yarn.lock | head -30
fi
echo -e "\n=== Check git diff or PR context to understand why preact was added ==="
# Look for any recent changes mentioning preact or `@falkordb/canvas`
git log -5 --oneline --all 2>/dev/null | head -10
</parameter>Repository: FalkorDB/QueryWeaver
Length of output: 834
🏁 Script executed:
#!/bin/bash
# Search more thoroughly for preact usage in the codebase
echo "=== Searching for any preact imports or references ==="
rg "preact" --type-list | head -5
rg "preact" -t js -t json -t ts -t tsx -t jsx -i 2>/dev/null || echo "No preact references found"
echo -e "\n=== Check vite or build config ==="
if [ -f app/vite.config.ts ]; then
echo "=== vite.config.ts content ==="
cat app/vite.config.ts
fi
if [ -f app/vite.config.js ]; then
echo "=== vite.config.js content ==="
cat app/vite.config.js
fi
echo -e "\n=== Check if preact is listed in package.json but actually not needed ==="
cd app
npm ls preact 2>&1 | head -20
</parameter>Repository: FalkorDB/QueryWeaver
Length of output: 1635
Remove the unnecessary preact dependency.
preact is not used anywhere in the codebase and is not required by @falkordb/canvas (which depends on react directly). It adds bundle bloat without purpose. Remove preact from package.json line 54.
Also note that @falkordb/canvas at 0.0.29 is a pre-1.0 release; expect potential breaking changes with future updates.
🤖 Prompt for AI Agents
In `@app/package.json` at line 14, Remove the unused "preact" dependency from
package.json: delete the "preact" entry so it no longer contributes to
install/bundle size; keep "@falkordb/canvas": "^0.0.29" as-is but be aware it's
pre-1.0 (no code changes to `@falkordb/canvas` required). Ensure package.json
still validates (commas, JSON structure) and run npm/yarn install and a quick
project build to confirm nothing breaks after removing "preact".
| useEffect(() => { | ||
| import('@falkordb/canvas').then(() => { | ||
| setCanvasLoaded(true); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Dynamic import has no error handling — a failed load leaves the user with no feedback.
If import('@falkordb/canvas') rejects (e.g. network issue, corrupt chunk), canvasLoaded stays false and the canvas never renders. The user sees "No schema data available" with no indication that the canvas library failed to load.
🛡️ Proposed fix
useEffect(() => {
- import('@falkordb/canvas').then(() => {
- setCanvasLoaded(true);
- });
+ import('@falkordb/canvas')
+ .then(() => setCanvasLoaded(true))
+ .catch((err) => {
+ console.error('Failed to load `@falkordb/canvas`', err);
+ toast({
+ title: 'Schema Viewer Error',
+ description: 'Failed to load graph visualization library',
+ variant: 'destructive',
+ });
+ });
}, []);🤖 Prompt for AI Agents
In `@app/src/components/schema/SchemaViewer.tsx` around lines 86 - 90, The dynamic
import in the useEffect that loads '@falkordb/canvas' lacks error handling, so
if import('@falkordb/canvas') rejects the component silently stays in the "not
loaded" state; update the effect around import('@falkordb/canvas') (the
useEffect block that calls setCanvasLoaded) to add a .catch handler (or
try/catch if using async) that logs the error and updates component state (e.g.,
setCanvasLoaded(false) plus a new setCanvasLoadError or similar boolean/string)
so the UI can show an error message or fallback UI when loading fails; ensure
you continue to setCanvasLoaded(true) on success and reference useEffect,
import('@falkordb/canvas'), setCanvasLoaded, and the new error state so
reviewers can locate the change.
| // Update links to use the new node IDs | ||
| data.links = data.links.map((link) => ({ | ||
| ...link, | ||
| source: oldIdToNewId.get(link.source) || link.source, | ||
| target: oldIdToNewId.get(link.target) || link.target, | ||
| })); |
There was a problem hiding this comment.
Link remapping fallback silently preserves the original string ID, causing a type mismatch.
When oldIdToNewId.get(link.source) returns undefined (i.e., a link references a node not present in the nodes array), the || fallback yields link.source which is a string — but SchemaLink.source is typed as number. This produces a silent type mismatch that will result in broken/orphaned links in the canvas.
Use a stricter approach that surfaces missing nodes:
🐛 Proposed fix
// Update links to use the new node IDs
- data.links = data.links.map((link) => ({
- ...link,
- source: oldIdToNewId.get(link.source) || link.source,
- target: oldIdToNewId.get(link.target) || link.target,
- }));
+ data.links = data.links.reduce<typeof data.links>((acc, link) => {
+ const source = oldIdToNewId.get(link.source);
+ const target = oldIdToNewId.get(link.target);
+ if (source !== undefined && target !== undefined) {
+ acc.push({ ...link, source, target });
+ } else {
+ console.warn('Dropping link with unknown node ID', link);
+ }
+ return acc;
+ }, []);📝 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.
| // Update links to use the new node IDs | |
| data.links = data.links.map((link) => ({ | |
| ...link, | |
| source: oldIdToNewId.get(link.source) || link.source, | |
| target: oldIdToNewId.get(link.target) || link.target, | |
| })); | |
| // Update links to use the new node IDs | |
| data.links = data.links.reduce<typeof data.links>((acc, link) => { | |
| const source = oldIdToNewId.get(link.source); | |
| const target = oldIdToNewId.get(link.target); | |
| if (source !== undefined && target !== undefined) { | |
| acc.push({ ...link, source, target }); | |
| } else { | |
| console.warn('Dropping link with unknown node ID', link); | |
| } | |
| return acc; | |
| }, []); |
🤖 Prompt for AI Agents
In `@app/src/components/schema/SchemaViewer.tsx` around lines 150 - 155, The
current remapping in data.links uses the || fallback which can return the
original string ID and cause a type mismatch for SchemaLink.source/target;
change the mapping to explicitly resolve newSource =
oldIdToNewId.get(link.source) and newTarget = oldIdToNewId.get(link.target) and
if either is undefined either (a) throw or log and remove the link so the
missing node is surfaced, or (b) raise a TypeError with details (include
link.id, link.source, link.target) so callers know about the orphaned link;
update the code that uses data.links (the mapping block referencing oldIdToNewId
and SchemaLink.source/target) to perform this explicit check instead of using ||
fallback.
| const handleCenter = useCallback(() => { | ||
| console.log("Stop"); | ||
|
|
There was a problem hiding this comment.
Remove debug console.log("Stop").
This is a leftover debug statement that shouldn't ship to production.
🧹 Proposed fix
const handleCenter = useCallback(() => {
- console.log("Stop");
-
const canvas = canvasRef.current;📝 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 handleCenter = useCallback(() => { | |
| const canvas = canvasRef.current; |
🤖 Prompt for AI Agents
In `@app/src/components/schema/SchemaViewer.tsx` around lines 189 - 191, Remove
the debug console.log("Stop") from the handleCenter callback in SchemaViewer
(the useCallback named handleCenter) so it no longer emits console output;
simply delete the console.log line and keep the existing logic of the callback
intact.
| {!loading && canvasLoaded && schemaData && schemaData.nodes.length > 0 && ( | ||
| <falkordb-canvas ref={canvasRef} node-mode='replace' /> | ||
| )} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for falkordb-canvas type declarations and JSX IntrinsicElements
rg -i "falkordb-canvas" --type ts --type tsx --type d.ts -C5Repository: FalkorDB/QueryWeaver
Length of output: 91
🏁 Script executed:
# Search for JSX.IntrinsicElements declarations in the codebase
rg "JSX\s*{\s*namespace|IntrinsicElements" --type ts --type tsx --type d.ts -C3Repository: FalkorDB/QueryWeaver
Length of output: 190
🏁 Script executed:
# Check for any global type definitions or d.ts files
find . -name "*.d.ts" -o -name "global.d.ts" -o -name "types.d.ts" | grep -v node_modules | head -20Repository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Check package.json for `@falkordb/canvas` dependency
cat package.json | grep -A5 -B5 "falkordb" || echo "Not found in package.json"Repository: FalkorDB/QueryWeaver
Length of output: 89
🏁 Script executed:
# Look for `@falkordb/canvas` in node_modules and check its type definitions
find . -path "*/node_modules/@falkordb/canvas*" -name "*.d.ts" -o -path "*/node_modules/@falkordb/canvas*" -name "package.json" 2>/dev/null | head -10Repository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Search for falkordb-canvas using correct file type
rg "falkordb-canvas" --type ts -C5Repository: FalkorDB/QueryWeaver
Length of output: 1856
🏁 Script executed:
# Search for JSX IntrinsicElements declarations
rg "IntrinsicElements" --type ts -C3Repository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Look for global type definitions
find . -name "*.d.ts" -not -path "*/node_modules/*" 2>/dev/nullRepository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Check the actual SchemaViewer.tsx file to see how it's importing/using falkordb-canvas
find . -name "SchemaViewer.tsx" -exec cat {} \; 2>/dev/nullRepository: FalkorDB/QueryWeaver
Length of output: 14812
🏁 Script executed:
# Search for `@falkordb` in package.json and related config files
find . -maxdepth 2 -name "package.json" -o -name "package-lock.json" | xargs grep -l "falkordb" 2>/dev/null || echo "No falkordb found"Repository: FalkorDB/QueryWeaver
Length of output: 126
🏁 Script executed:
# Check if there's a TypeScript errors in the project
ls -la tsconfig.json 2>/dev/null && cat tsconfig.json || echo "No tsconfig.json found"Repository: FalkorDB/QueryWeaver
Length of output: 86
🏁 Script executed:
# Check app/package.json for `@falkordb/canvas`
cat ./app/package.json | grep -A10 -B10 "falkordb\|@falkordb"Repository: FalkorDB/QueryWeaver
Length of output: 771
🏁 Script executed:
# Check if `@falkordb/canvas` is installed in node_modules
find ./app/node_modules -name "@falkordb" -type d 2>/dev/null || echo "Not found"Repository: FalkorDB/QueryWeaver
Length of output: 73
🏁 Script executed:
# Search for any JSX type augmentation or global declarations
rg "declare.*JSX\|namespace.*JSX\|IntrinsicElements" . --type ts -B2 -A5 2>/dev/null || echo "Not found"Repository: FalkorDB/QueryWeaver
Length of output: 73
🏁 Script executed:
# Check if the custom element might be defined elsewhere
rg "falkordb-canvas" . --type ts --type d.ts -B10 -A10 2>/dev/null | head -100Repository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Look for tsconfig in the app directory
find ./app -name "tsconfig.json" -o -name "tsconfig*.json" | head -5Repository: FalkorDB/QueryWeaver
Length of output: 159
🏁 Script executed:
# Check if there are any .d.ts files that might declare the element
find ./app -name "*.d.ts" -not -path "*/node_modules/*" 2>/dev/nullRepository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Check the main tsconfig.json in app
cat ./app/tsconfig.jsonRepository: FalkorDB/QueryWeaver
Length of output: 391
🏁 Script executed:
# Check if there's a vite-env.d.ts or similar
find ./app/src -name "*.d.ts" 2>/dev/nullRepository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Look for any env or global declaration files
find ./app -maxdepth 3 -name "*env.d.ts" -o -name "vite-env.d.ts" -o -name "global.d.ts"Repository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Search for any declarations that might handle falkordb-canvas
rg "falkordb" ./app --type ts -B5 -A5 2>/dev/null | head -150Repository: FalkorDB/QueryWeaver
Length of output: 5559
🏁 Script executed:
# Check what's actually exported from `@falkordb/canvas`
find ./app/node_modules/@falkordb -name "*.d.ts" 2>/dev/null | head -20Repository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Try to locate `@falkordb/canvas` package.json to check typings entry
find . -path "*/@falkordb/canvas*" -name "package.json" 2>/dev/nullRepository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Check the actual TypeScript compilation - look for any error logs or build outputs
find ./app -name "dist" -o -name "build" -o -name ".vite" | head -5Repository: FalkorDB/QueryWeaver
Length of output: 46
🏁 Script executed:
# Try a different approach - check if there are any vite or build configuration files
cat ./app/vite.config.ts 2>/dev/null || cat ./app/vite.config.js 2>/dev/null || echo "No vite config found"Repository: FalkorDB/QueryWeaver
Length of output: 837
🏁 Script executed:
# Search for any React type definitions that might augment JSX
find ./app -name "react.d.ts" -o -name "index.d.ts" | xargs grep -l "IntrinsicElements\|falkordb" 2>/dev/null || echo "None found"Repository: FalkorDB/QueryWeaver
Length of output: 74
🏁 Script executed:
# Check if `@falkordb/canvas` might declare elements via a global augmentation
rg "declare\s+global|module\s+[\'\"]@falkordb" ./app --type ts --type d.ts 2>/dev/null || echo "Not found"Repository: FalkorDB/QueryWeaver
Length of output: 73
🌐 Web query:
@falkordb/canvas npm package type definitions JSX IntrinsicElements
💡 Result:
@falkordb/canvas already ships TypeScript types and it declares the custom element for JSX.
- The package points TypeScript at
dist/index.d.tsvia"types": "dist/index.d.ts"(andexports["."].types). [1] - In
dist/index.d.tsit does adeclare global { namespace JSX { interface IntrinsicElements { ... }}}augmentation that adds the"falkordb-canvas"intrinsic element, plus aHTMLElementTagNameMapentry. [2] - The props it exposes on the JSX tag include:
'node-mode'?: CanvasRenderMode'link-mode'?: CanvasRenderMode[2]
CanvasRenderModeis defined as'before' | 'after' | 'replace'. [3]- TypeScript’s JSX intrinsic element checking works exactly via
JSX.IntrinsicElementsaugmentation. [4]
If TS/JSX still says "Property 'falkordb-canvas' does not exist on type JSX.IntrinsicElements'":
Make sure the package’s global augmentation is included by importing it somewhere that’s in your TS program, e.g. in your app entry or a global.d.ts:
import "@falkordb/canvas";This forces dist/index.d.ts (and its declare global { JSX.IntrinsicElements ... }) to be loaded. [2][4]
Sources: [1] package.json types/exports; [2] dist/index.d.ts global JSX + tag map; [3] dist/canvas-types.d.ts CanvasRenderMode; [4] TypeScript JSX docs on JSX.IntrinsicElements.
🏁 Script executed:
# Check if there's a compiled output or build artifacts
ls -la ./app/dist 2>/dev/null || echo "No dist directory"Repository: FalkorDB/QueryWeaver
Length of output: 81
🏁 Script executed:
# Check app/src for any setup or entry files that might have the import
cat ./app/src/main.tsx 2>/dev/null || cat ./app/src/index.tsx 2>/dev/null || echo "Not found"Repository: FalkorDB/QueryWeaver
Length of output: 896
🏁 Script executed:
# Check if there are any type declaration files in src that might augment JSX globally
find ./app/src -name "*.d.ts" -exec cat {} \;Repository: FalkorDB/QueryWeaver
Length of output: 46
Add a side-effect import of @falkordb/canvas to load JSX type declarations.
The code uses a type-only import (import type { ... } from '@falkordb/canvas'), which TypeScript erases at compile time and does not execute global side effects. The @falkordb/canvas package declares the falkordb-canvas JSX element via a global augmentation in its type definitions, but this augmentation is never loaded. This will produce a TypeScript compile error: Property 'falkordb-canvas' does not exist on type JSX.IntrinsicElements.
Add import '@falkordb/canvas'; (without the type keyword) at the top of the file to ensure the global JSX augmentation is loaded before the component uses the element.
🤖 Prompt for AI Agents
In `@app/src/components/schema/SchemaViewer.tsx` around lines 451 - 453, Add a
side-effect import to ensure the global JSX augmentation for the custom element
is loaded: at the top of SchemaViewer.tsx add import '@falkordb/canvas'; (no
`type`) so TypeScript includes the module at runtime and the JSX intrinsic
element falkordb-canvas is recognized when rendering the falkordb-canvas element
in the SchemaViewer component.
| MEMORY_TTL_SECONDS=604800 | ||
| ``` | ||
|
|
||
| The TTL is refreshed on every user interaction, so active users keep their memory. |
There was a problem hiding this comment.
Documentation may overstate the TTL refresh guarantee.
Line 69 says "The TTL is refreshed on every user interaction." As noted in the graphiti_tool.py review, _refresh_ttl() is only called inside MemoryTool.create(). If create() isn't invoked on every interaction, this statement is misleading. Please verify the call path and update the wording if needed.
🤖 Prompt for AI Agents
In `@README.md` at line 69, The README overstates TTL behavior — _refresh_ttl() is
only invoked from MemoryTool.create(), so update the sentence "The TTL is
refreshed on every user interaction" to accurately reflect that TTL is refreshed
only when MemoryTool.create() is called (or change the implementation to call
_refresh_ttl() on every interaction if you intend the original claim); reference
the _refresh_ttl() method and MemoryTool.create() in the updated wording so
readers know the exact trigger for TTL refresh.
There was a problem hiding this comment.
Summary
- Importance counts: BLOCKER 1 · CRITICAL 0 · MAJOR 6 · MINOR 0 · SUGGESTION 0 · PRAISE 0
- Key themes:
- Memory retention feature does not actually refresh or enforce TTL reliably, and failures currently degrade silently.
- Schema viewer refactor regresses basic rendering (missing labels) and link integrity after node ID remapping.
- Security middleware ordering and the accompanying HSTS tests need tightening—the header is not guaranteed to be applied, and the tests spin up the full app over HTTP with real DB connections.
Next steps
- Reorder the FastAPI middleware stack so
SecurityMiddlewarewraps all subsequent middleware, and adjust the HSTS tests to avoid real FalkorDB dependencies while validating HTTPS behavior. - Ensure every memory read/write path calls
_refresh_ttland surfaces TTL refresh failures instead of silently continuing when Redis/FalkorDB is unavailable. - Fix the schema canvas data mapping so node titles use real schema names and drop/log edges whose endpoints are missing after the ID remap.
Blocking issues remain (middleware ordering), so the review is left in COMMENT state pending fixes.
| 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) |
There was a problem hiding this comment.
[MAJOR]: _refresh_ttl only logs RedisError and then returns control to the caller, so a FalkorDB/Redis outage silently disables TTL enforcement—graphs never expire but nothing fails or alerts. That means the “memory auto-expiry” feature can be broken for weeks without anyone knowing. Please fail MemoryTool.create (or otherwise signal the failure) when the EXPIRE call raises so the request can retry/alert rather than silently degrading security/privacy guarantees.
| await driver.execute_query(f"CREATE VECTOR INDEX FOR (p:Query) ON (p.embeddings) OPTIONS {{dimension:{vector_size}, similarityFunction:'euclidean'}}") | ||
|
|
||
| if cls.MEMORY_TTL_SECONDS is not None: | ||
| await self._refresh_ttl() |
There was a problem hiding this comment.
[MAJOR]: The README now promises “The TTL is refreshed on every user interaction,” but _refresh_ttl() is only invoked once inside MemoryTool.create. After MEMORY_TTL_SECONDS elapses the user graph expires even if the user was actively chatting, which defeats the idle-based TTL requirement. Please refresh the TTL on every read/write path (e.g. search, add_new_memory, save_query_memory) so each interaction extends the expiry.
| ctx.textAlign = 'center'; | ||
| ctx.textBaseline = 'middle'; | ||
| ctx.fillText( | ||
| node.displayName[1], |
There was a problem hiding this comment.
[MAJOR]: Custom node rendering now uses node.displayName[1] for the header text, but the canvas data never sets a displayName array—nodes only have data.name. As a result every table renders without a label. Please render schemaNode.name (or node.data?.name) instead so titles show up again.
| // Update links to use the new node IDs | ||
| data.links = data.links.map((link) => ({ | ||
| ...link, | ||
| source: oldIdToNewId.get(link.source) || link.source, |
There was a problem hiding this comment.
[MAJOR]: When remapping nodes to numeric IDs, links fall back to the original string ID if oldIdToNewId has no match: oldIdToNewId.get(link.source) || link.source. That produces mixed string/number IDs in the same graph, so any link whose endpoint wasn�t mapped now references a non-existent node in the canvas and silently disappears. Instead, fail/skip/log these edges so the rendered graph stays consistent.
| # 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" |
There was a problem hiding this comment.
[BLOCKER]: Middleware is stacked LIFO, so adding SecurityMiddleware inside RequestValidationMiddleware.dispatch means it executes after ProxyHeadersMiddleware/SessionMiddleware have already built their response. Redirects, cookies, etc. leave the app before the HSTS header is injected, defeating the goal of globally enforcing HSTS. Please register the HSTS middleware at FastAPI app creation (before ProxyHeaders/Session) so every response carries the header.
| """ | ||
| import pytest | ||
| from fastapi.testclient import TestClient | ||
| from api.index import app |
There was a problem hiding this comment.
[MAJOR]: Importing app from api.index spins up the full production app (including FalkorDB/Redis connections) at import time. These tests will fail in CI or local dev whenever no database is running, even though the tests only need to inspect headers. Please instantiate a lightweight app for the test (e.g., call the factory with dependencies patched or provide a settings flag to skip DB initialization) so the HSTS tests don’t require a live FalkorDB instance.
| """Create a test client.""" | ||
| return TestClient(app) | ||
|
|
||
| def test_hsts_header_present(self, client): |
There was a problem hiding this comment.
[MAJOR]: These tests call TestClient(app) over HTTP and merely assert that the header exists. HSTS is ignored on insecure transport, so this test proves only that FastAPI can set a header—not that the middleware behaves correctly under HTTPS. Please adjust the client to use an HTTPS base URL or patch the middleware to emit the header only when request.url.scheme == 'https', and cover both the HTTPS and HTTP cases to ensure the production configuration is actually exercised.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Dockerfile`:
- Around line 43-50: Scope the "|| true" to only the removal step and add
--no-install-recommends to the install: change the RUN chain so apt-get update
is not short-circuited by the "|| true" (i.e., use 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), ensuring the "remove" fallback only applies to the
remove command and the install uses --no-install-recommends.
- Around line 69-70: The Dockerfile contains a redundant COPY . . (the later
COPY . . duplicates the earlier copy of the app/ directory); remove the second
COPY . . (or consolidate into a single targeted COPY that copies only the
necessary source files) and add a .dockerignore to exclude unnecessary context
(e.g., .git, node_modules, .venv, .env, app/dist) so builds are smaller and
faster; locate the duplicate COPY . . entry and either delete it or replace both
COPY operations with a single explicit COPY that targets the application source,
then add a .dockerignore file listing the unwanted files/directories.
| 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.
Shell operator precedence issue silently swallows apt-get update failure; also add --no-install-recommends.
Because && and || have equal precedence and are left-associative in shell, the expression apt-get update && apt-get remove -y nodejs || true && ... evaluates as ((apt-get update && apt-get remove ...) || true) && .... If apt-get update on line 43 fails, the || true catches it and the rest of the chain proceeds silently with a stale/missing package index. Scope the || true to only the remove command.
Additionally, apt-get install -y nodejs on line 48 is missing --no-install-recommends, which would pull in unnecessary recommended packages and increase image size (flagged by Trivy DS-0029).
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/* \
- && 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, Scope the "|| true" to only the removal
step and add --no-install-recommends to the install: change the RUN chain so
apt-get update is not short-circuited by the "|| true" (i.e., use 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), ensuring the "remove" fallback only applies
to the remove command and the install uses --no-install-recommends.
| # Copy application code | ||
| COPY . . |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if .dockerignore exists and what it excludes related to app/ build artifacts
if [ -f .dockerignore ]; then
echo "=== .dockerignore contents ==="
cat .dockerignore
else
echo "WARNING: No .dockerignore found"
fiRepository: FalkorDB/QueryWeaver
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check the Dockerfile around lines 69-70 for full context
echo "=== Dockerfile (full content) ==="
if [ -f Dockerfile ]; then
cat -n Dockerfile
else
echo "Dockerfile not found"
fi
echo ""
echo "=== .gitignore contents ==="
if [ -f .gitignore ]; then
cat .gitignore
else
echo "No .gitignore found"
fi
echo ""
echo "=== Check if app/node_modules or app/dist exist in repo ==="
git ls-files | grep -E '(app/node_modules|app/dist|app/build)' | head -20 || echo "No committed build artifacts or node_modules found"Repository: FalkorDB/QueryWeaver
Length of output: 3629
COPY . . on line 70 is redundant; the app/ directory was already copied on line 65.
The subsequent COPY . . will re-copy application code without risk of overwriting the frontend build artifacts (app/dist/), since .gitignore properly excludes both node_modules/ and /app/dist/ from the repository. However, a .dockerignore file would be beneficial to exclude unnecessary build context files (e.g., .git, .venv, .env) and improve build efficiency. Consider removing the redundant COPY . . or, if it's needed for Python application code, consolidate the two COPY operations and add a .dockerignore to exclude build directories, cache files, and version control artifacts.
🤖 Prompt for AI Agents
In `@Dockerfile` around lines 69 - 70, The Dockerfile contains a redundant COPY .
. (the later COPY . . duplicates the earlier copy of the app/ directory); remove
the second COPY . . (or consolidate into a single targeted COPY that copies only
the necessary source files) and add a .dockerignore to exclude unnecessary
context (e.g., .git, node_modules, .venv, .env, app/dist) so builds are smaller
and faster; locate the duplicate COPY . . entry and either delete it or replace
both COPY operations with a single explicit COPY that targets the application
source, then add a .dockerignore file listing the unwanted files/directories.
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores
Tests