Add Remote clients over REST #272
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds new Python and TypeScript clients with async HTTP/streaming support, documentation, configs, and tests. Updates Makefile and CI to install and test clients. Minor frontend TypeScript cleanups to unused variables and catches. Adjusts ESLint rules and gitignore entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer App
participant QC as QueryWeaverClient (Python/TS)
participant HTTP as HTTP Server (QueryWeaver)
Note over Dev,QC: Initialize client with baseUrl, optional apiToken
Dev->>QC: listSchemas()
QC->>HTTP: GET /graphs (Authorization?)
HTTP-->>QC: 200 JSON {graphs:[...]}
QC-->>Dev: string[]
Dev->>QC: query(graphId, chatData)
QC->>HTTP: POST /graphs/{id} (JSON body)
rect rgba(200, 230, 255, 0.3)
Note over QC,HTTP: Streaming response (NDJSON/chunked delimiter)
HTTP-->>QC: chunk1 | ... | final event
QC->>QC: Buffer, split by delimiter, parse JSON
end
QC-->>Dev: final event (object)
Dev->>QC: connectDatabase(dbUrl)
QC->>HTTP: POST /database (JSON)
HTTP-->>QC: stream chunks
QC-->>Dev: last parsed event
alt Non-2xx
HTTP-->>QC: Error status + JSON/text
QC-->>Dev: throw APIError(status, response)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks
✅ Passed checks (1 passed)
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 PR introduces a new asynchronous Python client for QueryWeaver and refactors server-side graph/database routes into core modules, aligning responsibilities and improving maintainability. It also bumps several dependencies and adjusts streaming behavior to be more consistent.
- Add Python aiohttp-based client with basic tests and README
- Refactor graph/database logic into api/core (text2sql, schema_loader, errors) and update routes to use these abstractions
- Update package versions, Dockerfile build deps, and minor frontend/TS fix
Reviewed Changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| server.json | Bump server and package versions; adjust name casing. |
| clients/python/queryweaver_client/* | New async Python client and package metadata. |
| clients/python/tests/test_client_basic.py | New tests for the Python client. |
| clients/python/README.md | Usage and development instructions for the client. |
| clients/python/Pipfile | Pipenv setup for the client (aiohttp, pytest-asyncio). |
| clients/python/setup.py | Packaging metadata for the client (install_requires, extras). |
| app/ts/modules/graphs.ts | Minor boolean cast fix for demo graph detection. |
| app/package.json | Dev dependency bumps (esbuild, eslint, TS ESLint). |
| api/routes/graphs.py | Route layer refactored to call core text2sql/schema_loader functions. |
| api/routes/database.py | Route now delegates to core schema_loader.load_database. |
| api/core/text2sql.py | New core module: query, schema retrieval, destructive ops, refresh, delete. |
| api/core/schema_loader.py | New core module: database connection streaming and listing databases. |
| api/core/errors.py | New custom exception classes. |
| api/core/init.py | Export core symbols for convenience. |
| Pipfile | Bump backend dependency versions. |
| Dockerfile | Add build-essential and clean up apt usage. |
| CODE_OF_CONDUCT.md | Add project Code of Conduct. |
| .github/wordlist.txt | Update wordlist for spellcheck. |
Files not reviewed (1)
- app/package-lock.json: Language not supported
|
@CodeRabbit review |
|
/review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
clients/python/queryweaver_client/client.py (1)
69-71: Don’t close caller-owned sessions
__aexit__closes the session unconditionally, so external sessions handed in by the caller get torn down unexpectedly. Track ownership and only close sessions the client created itself.- async def __aexit__(self, exc_type, exc_val, exc_tb): - if self._session and not self._session.closed: - await self._session.close() + async def __aexit__(self, exc_type, exc_val, exc_tb): + if self._owns_session and self._session and not self._session.closed: + await self._session.close()
🧹 Nitpick comments (3)
app/ts/modules/left_toolbar.ts (1)
33-35: Consider logging the error for debugging.The bare
catchblock suppresses any error that occurs during the initialsetOpencall. While the fallback tosetOpen(true)is safe, logging the error would aid debugging if media query matching fails unexpectedly.Apply this diff to add error logging:
- } catch { + } catch (err) { + console.warn('Media query matching failed, defaulting to open:', err); setOpen(true); }clients/python/Pipfile (1)
14-15: Loosen Python version constraint
In clients/python/Pipfile (lines 14–15), python_version is set to "3.12", but no 3.12-only syntax (e.g., PEP 695 generics) is used. Unless you require 3.12, consider broadening support to Python 3.8+.clients/python/README.md (1)
107-107: Optional: Consistent horizontal rule style.For consistency with line 97, consider using the same horizontal rule style (
-----------------------) instead of (-----------).Apply this diff:
-## Want more? ------------ +## Want more? +-----------------------
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
clients/python/Pipfile.lockis excluded by!**/*.lockclients/ts/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
.github/workflows/tests.yml(5 hunks).gitignore(2 hunks)Makefile(3 hunks)app/eslint.config.cjs(1 hunks)app/ts/modules/graph_select.ts(1 hunks)app/ts/modules/left_toolbar.ts(2 hunks)app/ts/modules/messages.ts(1 hunks)app/ts/modules/modals.ts(1 hunks)app/ts/modules/tokens.ts(1 hunks)clients/python/Pipfile(1 hunks)clients/python/README.md(1 hunks)clients/python/queryweaver_client/__init__.py(1 hunks)clients/python/queryweaver_client/client.py(1 hunks)clients/python/setup.py(1 hunks)clients/python/tests/test_client_basic.py(1 hunks)clients/ts/.eslintrc.json(1 hunks)clients/ts/.gitignore(1 hunks)clients/ts/README.md(1 hunks)clients/ts/jest.config.js(1 hunks)clients/ts/package.json(1 hunks)clients/ts/src/client.ts(1 hunks)clients/ts/src/index.ts(1 hunks)clients/ts/src/types.ts(1 hunks)clients/ts/tests/client.test.ts(1 hunks)clients/ts/tsconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T15:24:21.682Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-02T15:24:21.682Z
Learning: Applies to tests/e2e/test_*.py : E2E test files should be placed under tests/e2e/test_*.py for Playwright runs
Applied to files:
.github/workflows/tests.yml
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
clients/python/README.md
107-107: Horizontal rule style
Expected: -----------------------; Actual: -----------
(MD035, hr-style)
🪛 Ruff (0.13.2)
clients/python/tests/test_client_basic.py
46-46: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Unused method argument: url
(ARG002)
75-75: Unused method argument: timeout
(ARG002)
87-87: Unused method argument: json
(ARG002)
87-87: Unused method argument: timeout
(ARG002)
117-117: Unused method argument: url
(ARG002)
117-117: Unused method argument: timeout
(ARG002)
183-183: Unused method argument: url
(ARG002)
183-183: Unused method argument: timeout
(ARG002)
197-197: Possible hardcoded password assigned to argument: "api_token"
(S106)
clients/python/queryweaver_client/client.py
83-83: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (19)
app/ts/modules/tokens.ts (1)
141-141: LGTM: Unused error variable removed.Dropping the unused
errparameter is a valid cleanup that aligns with modern JavaScript/TypeScript best practices. The error handling behavior remains unchanged.clients/ts/.gitignore (1)
1-1: LGTM!Standard practice to exclude the TypeScript build output directory from version control.
.gitignore (1)
4-4: LGTM!Appropriate addition to prevent Python egg-info metadata (generated during packaging) from being committed.
app/ts/modules/modals.ts (1)
270-270: LGTM!Removing the explicit
thistype annotation relaxes TypeScript strictness while preserving runtime behavior, asthisis still correctly bound byaddEventListener.app/ts/modules/left_toolbar.ts (1)
57-57: LGTM!Removing the unused event parameter aligns with the ESLint configuration (
argsIgnorePattern: '^_') and improves code clarity.app/ts/modules/messages.ts (1)
77-77: LGTM!Removing the unused
indexparameter improves clarity and aligns with the project's ESLint configuration.clients/python/Pipfile (1)
1-15: LGTM overall!The Pipfile appropriately declares runtime dependencies (aiohttp), an editable local package, and testing dependencies (pytest, pytest-asyncio) consistent with the new async Python client implementation.
app/eslint.config.cjs (1)
23-23: LGTM!Adding
argsIgnorePattern: '^_'to theno-unused-varsrule is a standard practice that allows developers to explicitly mark unused parameters with a leading underscore, improving code clarity while maintaining linter hygiene. This aligns with the TypeScript client ESLint configuration.clients/ts/package.json (3)
1-36: LGTM overall!The package.json establishes a well-structured TypeScript client package with appropriate metadata, entry points, scripts, and development tooling. The structure aligns with the new client implementation and test coverage introduced in the PR.
33-35: Confirm Node ≥18 requirement
Client code uses nativefetchandAbortSignal.timeout, which are only supported in Node 18+. Keeping"engines": {"node": ">=18.0.0"}is correct.
32-32: No external runtime dependencies found
All imports and require calls in clients/ts/src are relative; thedependenciesfield may safely remain empty.clients/ts/.eslintrc.json (1)
1-23: LGTM!The ESLint configuration is well-structured and appropriate for a TypeScript client project. The rule allowing unused parameters prefixed with
_aligns with the callback naming conventions used elsewhere in the codebase.clients/python/setup.py (1)
1-52: LGTM!The package configuration is complete and correct. The runtime dependency on
aiohttpand development extras includingpytest-asyncioproperly support the async client implementation and test suite.app/ts/modules/graph_select.ts (1)
25-25: LGTM!The callback parameter rename to
_correctly signals intentionally unused parameters and aligns with the ESLint convention introduced in the TypeScript client configuration.clients/ts/tsconfig.json (1)
1-28: LGTM!The TypeScript configuration is comprehensive and follows best practices. Strict type checking, declaration generation, and proper include/exclude patterns provide a solid foundation for the client library.
clients/ts/jest.config.js (1)
1-15: LGTM!The Jest configuration is well-structured for a TypeScript project. The coverage collection settings and test patterns are appropriate for the client library test suite.
Makefile (1)
1-90: Well-structured modularization of build targets.The new targets (
install-server,install-clients,test-clients) properly separate server and client dependency management and testing, improving the development workflow and CI integration..github/workflows/tests.yml (2)
40-42: LGTM!The workflow consolidation to use Makefile targets (
install-server,test-unit,install-playwright,test-e2e) improves maintainability and aligns with the modularized build system.Also applies to: 52-52, 85-91, 107-107
121-148: Well-structured client testing job.The new
client-testsjob properly sets up both Python 3.11 and Node.js 18 environments and runs client tests independently from server tests, providing good separation of concerns.
| "api-client" | ||
| ], | ||
| "author": "QueryWeaver Team", | ||
| "license": "MIT", |
There was a problem hiding this comment.
License is MIT here, but the Python client's setup.py classifier declares AGPLv3. Please align licenses between clients (and the repository) to avoid confusion, or clarify the intended license for each client.
| "license": "MIT", | |
| "license": "AGPL-3.0", |
| classifiers=[ | ||
| "Development Status :: 4 - Beta", | ||
| "Intended Audience :: Developers", | ||
| "License :: OSI Approved :: GNU Affero General Public License v3", |
There was a problem hiding this comment.
This declares AGPLv3, while the TypeScript client declares MIT in package.json and README; please reconcile the license across client packages to ensure consistency.
| "License :: OSI Approved :: GNU Affero General Public License v3", | |
| "License :: OSI Approved :: MIT License", |
| "experimentalDecorators": true, | ||
| "emitDecoratorMetadata": true, | ||
| "types": ["node", "jest"] | ||
| }, |
There was a problem hiding this comment.
[nitpick] Including jest types in the main tsconfig can leak test globals into the library build. Consider moving Jest types to a dedicated test tsconfig (e.g., tsconfig.spec.json) or configuring ts-jest to supply them, and keep the production build free of test-only types.
| return self | ||
|
|
||
| async def __aexit__(self, exc_type, exc_val, exc_tb): | ||
| if self._session and not self._session.closed: |
There was a problem hiding this comment.
This will close a user-provided session on exit. Since you track ownership with _owns_session, only close the session if it was created by the client (self._owns_session is True). Suggested fix: check self._owns_session before closing.
| if self._session and not self._session.closed: | |
| if self._owns_session and self._session and not self._session.closed: |
| this.defaultHeaders = { | ||
| 'Accept': 'application/json', | ||
| 'Content-Type': 'application/json', | ||
| }; | ||
|
|
||
| if (this.apiToken) { | ||
| this.defaultHeaders['Authorization'] = `Bearer ${this.apiToken}`; | ||
| } |
There was a problem hiding this comment.
Setting Content-Type: application/json on all requests (including GET) can trigger unnecessary CORS preflight requests in browsers and increase latency. Consider removing Content-Type from defaultHeaders and only setting it on requests with a body (e.g., POST), or constructing headers per call.
| "Development Status :: 4 - Beta", | ||
| "Intended Audience :: Developers", | ||
| "License :: OSI Approved :: GNU Affero General Public License v3", | ||
| "Programming Language :: Python :: 3", |
There was a problem hiding this comment.
The Python client declares AGPLv3 in its classifiers, while the TypeScript client package.json uses MIT. Please confirm and align the license across clients (and include the correct license file if needed) to avoid confusion for users.
Fix #271
Summary by CodeRabbit