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
📝 WalkthroughWalkthroughAdds Dependabot GitHub Actions config, injects an HSTS Strict-Transport-Security header in SecurityMiddleware, adds tests for that header, updates frontend dependencies (adds Changes
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.
|
|
||
| version: 2 | ||
| updates: | ||
| - package-ecosystem: "github-actions" |
There was a problem hiding this comment.
[IMPORTANCE]: major – The new github-actions entry omits a target-branch, so Dependabot will open workflow updates directly against main while the existing pip/npm configs are gated through staging. Please add target-branch: "staging" under this entry so all automation follows the same staging-first flow and avoids bypassing your promotion process.
…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
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', + }); + }); }, []);
| "preview": "vite preview" | ||
| }, | ||
| "dependencies": { | ||
| "@falkordb/canvas": "^0.0.29", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if preact is actually imported/used directly in source
echo "=== Checking direct preact imports in source ==="
rg "from ['\"]preact" app/src/ 2>/dev/null || echo "No direct preact imports found"
echo ""
echo "=== Checking for preact usage in app/src ==="
rg "preact" app/src/ 2>/dev/null || echo "No preact references found"
echo ""
echo "=== Checking app/package.json for dependencies ==="
grep -A 2 "@falkordb/canvas" app/package.json
grep -A 2 "preact" app/package.jsonRepository: FalkorDB/QueryWeaver
Length of output: 470
🏁 Script executed:
#!/bin/bash
# Check package-lock.json for `@falkordb/canvas` and its dependencies
echo "=== Checking `@falkordb/canvas` in package-lock.json ==="
jq '.packages["node_modules/@falkordb/canvas"] | {version, peerDependencies, dependencies}' app/package-lock.json 2>/dev/null || echo "Could not parse package-lock.json or `@falkordb/canvas` not found"
echo ""
echo "=== Checking if preact is a transitive dependency of `@falkordb/canvas` ==="
jq '.packages | to_entries[] | select(.value.dependencies["preact"] != null) | {key: .key, hasPpreact: true}' app/package-lock.json 2>/dev/null | head -20 || echo "Checking alternative method..."
echo ""
echo "=== Checking complete package.json to see context ==="
cat app/package.json | jq '.dependencies | keys' 2>/dev/null | head -30Repository: FalkorDB/QueryWeaver
Length of output: 1373
🏁 Script executed:
#!/bin/bash
# Check if float-tooltip is used in the codebase
echo "=== Checking if float-tooltip is imported/used ==="
rg "float-tooltip" app/src/ 2>/dev/null || echo "No float-tooltip imports found"
echo ""
echo "=== Checking if float-tooltip is a dependency of `@falkordb/canvas` ==="
jq '.packages["node_modules/float-tooltip"]' app/package-lock.json 2>/dev/null | head -30
echo ""
echo "=== Checking all packages that depend on preact ==="
jq '.packages | to_entries[] | select(.value.dependencies["preact"] != null or .value.peerDependencies["preact"] != null) | .key' app/package-lock.json 2>/dev/nullRepository: FalkorDB/QueryWeaver
Length of output: 680
Remove unnecessary preact direct dependency; it's only required transitively by float-tooltip. Also verify stability of @falkordb/canvas@0.0.29 (early version).
The preact@10.28.2 package is listed as a direct dependency but is not imported or used anywhere in app/src. It's only required as a transitive dependency via float-tooltip, which is pulled in by other libraries. Direct dependencies should only include packages actually used in the codebase.
The @falkordb/canvas@0.0.29 version is a pre-release (0.0.x), which may have stability concerns. Verify that this early version is intentional and stable enough for production use.
🤖 Prompt for AI Agents
In `@app/package.json` at 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.
| // 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.
Type mismatch in fallback values.
If a link references a node ID that wasn't mapped, the fallback || link.source / || link.target returns the original string value, but SchemaLink expects number for both properties. This could cause runtime issues when the canvas processes links with string IDs.
🔧 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
+ .map((link) => {
+ const source = oldIdToNewId.get(link.source);
+ const target = oldIdToNewId.get(link.target);
+ if (source === undefined || target === undefined) {
+ console.warn('Link references unknown node:', link);
+ return null;
+ }
+ return { ...link, source, target };
+ })
+ .filter((link): link is NonNullable<typeof link> => link !== null);🤖 Prompt for AI Agents
In `@app/src/components/schema/SchemaViewer.tsx` around lines 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).
| 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
Summary by CodeRabbit