Browser edge compute optimization#17
Conversation
- Create src/lib/onnxWorker.ts that loads onnxruntime-web and runs inference off the main thread - Rewrite ONNXEmbedder in ragClient.ts to communicate with the worker via postMessage with request IDs - Keep hashEmbedding fallback when no model URL is configured
- Add LazyRechartsChart and LazyChartjsChart wrappers that use React.lazy - Update charts/index.ts to export lazy wrappers instead of direct imports - Legacy chart libraries now load in separate async chunks
- do_get and get_flight_info now accept SQL statements (ticket/command prefixed with 'SQL ') and execute them against DuckDB via Arrow - Flight SQL clients can query telemetry catalogs directly without hitting the HTTP REST layer - Preserves backward compatibility with existing dataset-name tickets
- Swap redis:7-alpine for valkey/valkey:8-alpine in docker-compose - Update backend config defaults to valkey:// URLs - Update redis-exporter target and k8s secret key references - redis-py client remains compatible because Valkey speaks RESP
- New LLMGateway service in llm_gateway.py wraps LiteLLM calls - Config supports Portkey and Langfuse API keys and base URLs - LLMService routes completions through the gateway when configured
- New SemanticLayer service validates queries against a whitelist of measures/dimensions and compiles safe parameterized SQL - Agent semantic_query tool accepts structured JSON instead of raw SQL - Updated AIAgentService.SYSTEM_INSTRUCTION to prefer semantic queries
- Config flags TEMPORAL_ENABLED, TEMPORAL_HOST, TEMPORAL_NAMESPACE - New temporal_app.py defines DashboardGenerationWorkflow and TelemetrySyncWorkflow with activity stubs - docker-compose adds temporal service - k8s adds gridify-temporal-worker Deployment
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR adds semantic analytics querying, Flight SQL support, configurable LLM and Temporal integrations, Valkey deployment support, lazy-loaded charts, and Web Worker-based ONNX embedding execution. ChangesSemantic analytics and Flight SQL
LLM gateway routing
Temporal and Valkey runtime
Frontend loading and embeddings
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AIAgentService
participant SemanticLayer
participant DuckDB
AIAgentService->>SemanticLayer: validate structured query
SemanticLayer-->>AIAgentService: validated SemanticQuery
AIAgentService->>SemanticLayer: compile query to SQL
SemanticLayer-->>AIAgentService: safe SQL
AIAgentService->>DuckDB: execute SQL
DuckDB-->>AIAgentService: row count and sample
sequenceDiagram
participant TemporalWorker
participant DashboardGenerationWorkflow
participant ValidationActivity
participant InsightsActivity
TemporalWorker->>DashboardGenerationWorkflow: run query and context
DashboardGenerationWorkflow->>ValidationActivity: validate query
ValidationActivity-->>DashboardGenerationWorkflow: validation result
DashboardGenerationWorkflow->>InsightsActivity: generate insights
InsightsActivity-->>DashboardGenerationWorkflow: insights
DashboardGenerationWorkflow-->>TemporalWorker: workflow result
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds browser-side and backend execution optimizations. The main changes are:
Confidence Score: 4/5The raw Flight SQL paths, semantic SQL compiler, Portkey gateway path, and ONNX worker fallback need fixes before merging.
backend/app/services/arrow_service.py, backend/app/services/semantic_layer.py, backend/app/services/llm_gateway.py, src/lib/ragClient.ts
|
| Filename | Overview |
|---|---|
| backend/app/services/arrow_service.py | Adds Flight SQL execution paths that need auth and statement restrictions. |
| backend/app/services/semantic_layer.py | Adds semantic SQL compilation with clause-order and numeric-filter safety bugs. |
| backend/app/services/llm_gateway.py | Adds gateway routing, with a broken synchronous contract in the Portkey path. |
| src/lib/ragClient.ts | Moves ONNX inference into a worker, but init failures can disable fallback embedding. |
| src/components/charts/index.ts | Switches chart barrel exports to lazy wrapper names and drops previous component names. |
| backend/temporal_app.py | Adds optional Temporal workflow stubs. |
| docker-compose.yml | Switches Redis service config to Valkey and adds a Temporal service. |
| k8s/deployment.yaml | Updates Valkey secret keys and adds a Temporal worker deployment. |
Prompt To Fix All With AI
Fix the following 7 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 7
backend/app/services/arrow_service.py:168-174
**Flight Descriptor Executes Raw SQL**
When this Flight server is launched, any client that can reach `0.0.0.0:8815` can send a descriptor command starting with `SQL ` and the remainder is executed on the shared DuckDB connection. This bypasses the FastAPI guardrails and can run DDL or DML, including against attached PostgreSQL tables.
### Issue 2 of 7
backend/app/services/arrow_service.py:196-202
**Flight Ticket Executes Raw SQL**
A client can send a Flight ticket beginning with `SQL ` and this path sends the rest directly to `db.query_to_arrow(sql)`. Because tickets are accepted without auth, statement checks, or a read-only guard, reachable clients can mutate or drop data through the Flight port.
### Issue 3 of 7
backend/app/services/semantic_layer.py:204-213
**Filters Follow Grouping Clause**
A valid semantic query with any dimension and filter is emitted as `FROM telemetry GROUP BY ... WHERE ...`. SQL expects filtering before grouping, so normal requests like grouping by `device_id` with a `status` filter fail at execution instead of returning results.
### Issue 4 of 7
backend/app/services/semantic_layer.py:240-246
**Numeric Filters Remain Injectable**
`_validate_filter` accepts any value type for `gt`, `gte`, `lt`, and `lte`, and these branches paste the value into SQL without escaping. A semantic filter such as `score gt "0 OR 1=1"` compiles into executable SQL that changes the predicate instead of treating the value as data.
### Issue 5 of 7
backend/app/services/llm_gateway.py:52-57
**Portkey Returns Unawaited Coroutine**
The Portkey branch stores `acompletions` as the completion function, but `complete()` calls it synchronously and `LLMService` parses the returned object immediately. With `LLM_GATEWAY_PROVIDER=portkey`, this can hand `_extract_content` a coroutine instead of a response, producing empty completions rather than a real model answer.
### Issue 6 of 7
src/lib/ragClient.ts:130-136
**Failed Worker Init Looks Ready**
If the worker posts an `error` while loading the ONNX model, this rejects the init request but resolves the outer loading promise and leaves `this.worker` set. `ready()` then returns true, and later `embed()` skips the hash fallback and sends work to a worker with no session, causing embedding requests to reject.
### Issue 7 of 7
src/components/charts/index.ts:1-2
**Chart Barrel Drops Exports**
The barrel no longer exports the existing `RechartsChart` and `ChartjsChart` names. Code importing either component from `components/charts` will fail to compile after this change, even though the underlying default-exported components still exist.
Reviews (1): Last reviewed commit: "Fix ONNX worker tensor type cast for onn..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if command and command.startswith(b"SQL "): | ||
| sql = command[4:].decode("utf-8", errors="replace").strip() | ||
| if not sql: | ||
| raise ValueError("Empty SQL command") | ||
| from app.services.duckdb_service import get_duckdb_service | ||
| db = get_duckdb_service() | ||
| table = db.query_to_arrow(sql) |
There was a problem hiding this comment.
When this Flight server is launched, any client that can reach 0.0.0.0:8815 can send a descriptor command starting with SQL and the remainder is executed on the shared DuckDB connection. This bypasses the FastAPI guardrails and can run DDL or DML, including against attached PostgreSQL tables.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/app/services/arrow_service.py
Line: 168-174
Comment:
**Flight Descriptor Executes Raw SQL**
When this Flight server is launched, any client that can reach `0.0.0.0:8815` can send a descriptor command starting with `SQL ` and the remainder is executed on the shared DuckDB connection. This bypasses the FastAPI guardrails and can run DDL or DML, including against attached PostgreSQL tables.
How can I resolve this? If you propose a fix, please make it concise.| if ticket_str.startswith("SQL "): | ||
| sql = ticket_str[4:].strip() | ||
| if not sql: | ||
| raise ValueError("Empty SQL ticket") | ||
| from app.services.duckdb_service import get_duckdb_service | ||
| db = get_duckdb_service() | ||
| table = db.query_to_arrow(sql) |
There was a problem hiding this comment.
A client can send a Flight ticket beginning with SQL and this path sends the rest directly to db.query_to_arrow(sql). Because tickets are accepted without auth, statement checks, or a read-only guard, reachable clients can mutate or drop data through the Flight port.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/app/services/arrow_service.py
Line: 196-202
Comment:
**Flight Ticket Executes Raw SQL**
A client can send a Flight ticket beginning with `SQL ` and this path sends the rest directly to `db.query_to_arrow(sql)`. Because tickets are accepted without auth, statement checks, or a read-only guard, reachable clients can mutate or drop data through the Flight port.
How can I resolve this? If you propose a fix, please make it concise.| for d in query.dimensions: | ||
| dim = _ALLOWED_DIMENSIONS[d] | ||
| select_parts.append(dim.sql) | ||
| group_by_parts.append(dim.sql) | ||
|
|
||
| sql = f"SELECT {', '.join(select_parts)} FROM {table}" | ||
|
|
||
| if group_by_parts: | ||
| sql += f" GROUP BY {', '.join(group_by_parts)}" | ||
|
|
There was a problem hiding this comment.
A valid semantic query with any dimension and filter is emitted as FROM telemetry GROUP BY ... WHERE .... SQL expects filtering before grouping, so normal requests like grouping by device_id with a status filter fail at execution instead of returning results.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/app/services/semantic_layer.py
Line: 204-213
Comment:
**Filters Follow Grouping Clause**
A valid semantic query with any dimension and filter is emitted as `FROM telemetry GROUP BY ... WHERE ...`. SQL expects filtering before grouping, so normal requests like grouping by `device_id` with a `status` filter fail at execution instead of returning results.
How can I resolve this? If you propose a fix, please make it concise.| return f"{field} > {value}" | ||
| if operator == "gte": | ||
| return f"{field} >= {value}" | ||
| if operator == "lt": | ||
| return f"{field} < {value}" | ||
| if operator == "lte": | ||
| return f"{field} <= {value}" |
There was a problem hiding this comment.
_validate_filter accepts any value type for gt, gte, lt, and lte, and these branches paste the value into SQL without escaping. A semantic filter such as score gt "0 OR 1=1" compiles into executable SQL that changes the predicate instead of treating the value as data.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/app/services/semantic_layer.py
Line: 240-246
Comment:
**Numeric Filters Remain Injectable**
`_validate_filter` accepts any value type for `gt`, `gte`, `lt`, and `lte`, and these branches paste the value into SQL without escaping. A semantic filter such as `score gt "0 OR 1=1"` compiles into executable SQL that changes the predicate instead of treating the value as data.
How can I resolve this? If you propose a fix, please make it concise.| import portkey_ai # type: ignore | ||
|
|
||
| portkey_ai.api_key = settings.LLM_GATEWAY_PORTKEY_API_KEY | ||
| if settings.LLM_GATEWAY_BASE_URL: | ||
| portkey_ai.api_base = settings.LLM_GATEWAY_BASE_URL | ||
| return portkey_ai.Completions.acompletions |
There was a problem hiding this comment.
The Portkey branch stores acompletions as the completion function, but complete() calls it synchronously and LLMService parses the returned object immediately. With LLM_GATEWAY_PROVIDER=portkey, this can hand _extract_content a coroutine instead of a response, producing empty completions rather than a real model answer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/app/services/llm_gateway.py
Line: 52-57
Comment:
**Portkey Returns Unawaited Coroutine**
The Portkey branch stores `acompletions` as the completion function, but `complete()` calls it synchronously and `LLMService` parses the returned object immediately. With `LLM_GATEWAY_PROVIDER=portkey`, this can hand `_extract_content` a coroutine instead of a response, producing empty completions rather than a real model answer.
How can I resolve this? If you propose a fix, please make it concise.| if (type === "ready" || type === "result") { | ||
| pending.resolve(embeddings ?? []); | ||
| if (type === "ready") resolve(); | ||
| } else if (type === "error") { | ||
| pending.reject(new Error(error ?? "ONNX worker error")); | ||
| if (this.loading) resolve(); | ||
| } |
There was a problem hiding this comment.
If the worker posts an error while loading the ONNX model, this rejects the init request but resolves the outer loading promise and leaves this.worker set. ready() then returns true, and later embed() skips the hash fallback and sends work to a worker with no session, causing embedding requests to reject.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/ragClient.ts
Line: 130-136
Comment:
**Failed Worker Init Looks Ready**
If the worker posts an `error` while loading the ONNX model, this rejects the init request but resolves the outer loading promise and leaves `this.worker` set. `ready()` then returns true, and later `embed()` skips the hash fallback and sends work to a worker with no session, causing embedding requests to reject.
How can I resolve this? If you propose a fix, please make it concise.| export { LazyRechartsChart } from './LazyRechartsChart'; | ||
| export { LazyChartjsChart } from './LazyChartjsChart'; |
There was a problem hiding this comment.
The barrel no longer exports the existing RechartsChart and ChartjsChart names. Code importing either component from components/charts will fail to compile after this change, even though the underlying default-exported components still exist.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/charts/index.ts
Line: 1-2
Comment:
**Chart Barrel Drops Exports**
The barrel no longer exports the existing `RechartsChart` and `ChartjsChart` names. Code importing either component from `components/charts` will fail to compile after this change, even though the underlying default-exported components still exist.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several key enhancements, including an LLM gateway abstraction supporting Portkey and Langfuse, a structured semantic query layer for safer English-to-SQL translation, Flight SQL support in the Arrow service, optional Temporal workflows for durable execution, lazy-loaded chart components, and offloading ONNX inference to a dedicated Web Worker. However, several critical issues were identified in the review: the semantic layer compiles invalid SQL by placing GROUP BY before WHERE and is vulnerable to SQL injection due to unvalidated numeric filter values; the LLM gateway incorrectly calls an asynchronous Portkey method synchronously; Celery/Kombu configuration uses an unsupported valkey:// scheme; the Temporal worker deployment attempts to run a non-existent module; a race condition exists in the ONNX embedder initialization; the ONNX worker passes plain objects instead of ort.Tensor instances; and the Flight SQL endpoint uses a non-routable 0.0.0.0 address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if group_by_parts: | ||
| sql += f" GROUP BY {', '.join(group_by_parts)}" | ||
|
|
||
| where_clauses: List[str] = [] | ||
| for f in query.filters: | ||
| where_clauses.append(self._render_filter(f)) | ||
|
|
||
| if where_clauses: | ||
| sql += f" WHERE {' AND '.join(where_clauses)}" |
There was a problem hiding this comment.
In SQL, the WHERE clause must precede the GROUP BY clause. Currently, if a query contains both dimensions (which triggers GROUP BY) and filters (which triggers WHERE), the compiled SQL will place GROUP BY before WHERE, resulting in a syntax error in DuckDB. Rearrange the compilation order so that the WHERE clause is appended before the GROUP BY clause.
| if group_by_parts: | |
| sql += f" GROUP BY {', '.join(group_by_parts)}" | |
| where_clauses: List[str] = [] | |
| for f in query.filters: | |
| where_clauses.append(self._render_filter(f)) | |
| if where_clauses: | |
| sql += f" WHERE {' AND '.join(where_clauses)}" | |
| where_clauses: List[str] = [] | |
| for f in query.filters: | |
| where_clauses.append(self._render_filter(f)) | |
| if where_clauses: | |
| sql += f" WHERE {' AND '.join(where_clauses)}" | |
| if group_by_parts: | |
| sql += f" GROUP BY {', '.join(group_by_parts)}" |
| if operator == "between" and ( | ||
| not isinstance(value, list) or len(value) != 2 | ||
| ): | ||
| raise SemanticQueryError("'between' filter requires a 2-element value array") | ||
| if operator == "in" and not isinstance(value, list): | ||
| raise SemanticQueryError("'in' filter requires a value array") |
There was a problem hiding this comment.
The semantic layer does not validate the type of value for numeric operators (gt, gte, lt, lte, between). Since these values are directly interpolated into the SQL string without quotes or escaping, this introduces a SQL injection vulnerability if a malicious string is passed. Validate that the value is a number (int or float) for these operators.
| if operator == "between" and ( | |
| not isinstance(value, list) or len(value) != 2 | |
| ): | |
| raise SemanticQueryError("'between' filter requires a 2-element value array") | |
| if operator == "in" and not isinstance(value, list): | |
| raise SemanticQueryError("'in' filter requires a value array") | |
| if operator in ("gt", "gte", "lt", "lte") and not isinstance(value, (int, float)): | |
| raise SemanticQueryError(f"Value for operator '{operator}' must be a number") | |
| if operator == "between" and ( | |
| not isinstance(value, list) or len(value) != 2 or not all(isinstance(v, (int, float)) for v in value) | |
| ): | |
| raise SemanticQueryError("'between' filter requires a 2-element numeric array") | |
| if operator == "in" and not isinstance(value, list): | |
| raise SemanticQueryError("'in' filter requires a value array") |
| if self.provider == "portkey" and settings.LLM_GATEWAY_PORTKEY_API_KEY: | ||
| logger.info("Routing LLM calls through Portkey gateway") | ||
| try: | ||
| import portkey_ai # type: ignore | ||
|
|
||
| portkey_ai.api_key = settings.LLM_GATEWAY_PORTKEY_API_KEY | ||
| if settings.LLM_GATEWAY_BASE_URL: | ||
| portkey_ai.api_base = settings.LLM_GATEWAY_BASE_URL | ||
| return portkey_ai.Completions.acompletions |
There was a problem hiding this comment.
portkey_ai.Completions.acompletions is an asynchronous function, but LLMService.complete is synchronous. Calling an async function synchronously will return a coroutine object instead of the response, causing a runtime error when extracting content. Use the synchronous portkey_ai.Completions.create method instead.
| if self.provider == "portkey" and settings.LLM_GATEWAY_PORTKEY_API_KEY: | |
| logger.info("Routing LLM calls through Portkey gateway") | |
| try: | |
| import portkey_ai # type: ignore | |
| portkey_ai.api_key = settings.LLM_GATEWAY_PORTKEY_API_KEY | |
| if settings.LLM_GATEWAY_BASE_URL: | |
| portkey_ai.api_base = settings.LLM_GATEWAY_BASE_URL | |
| return portkey_ai.Completions.acompletions | |
| if self.provider == "portkey" and settings.LLM_GATEWAY_PORTKEY_API_KEY: | |
| logger.info("Routing LLM calls through Portkey gateway") | |
| try: | |
| import portkey_ai # type: ignore | |
| portkey_ai.api_key = settings.LLM_GATEWAY_PORTKEY_API_KEY | |
| if settings.LLM_GATEWAY_BASE_URL: | |
| portkey_ai.api_base = settings.LLM_GATEWAY_BASE_URL | |
| return portkey_ai.Completions.create |
| VALKEY_URL: str = "valkey://localhost:6379/0" | ||
| REDIS_URL: str = "valkey://localhost:6379/0" | ||
| CELERY_BROKER_URL: str = "valkey://localhost:6379/0" | ||
| CELERY_RESULT_BACKEND: str = "valkey://localhost:6379/0" |
There was a problem hiding this comment.
Celery/Kombu and standard redis-py do not support the valkey:// URL scheme out of the box. Attempting to use valkey:// will cause Celery to fail to start with a scheme parsing error. Since Valkey is fully Redis-compatible and speaks the RESP protocol, use the redis:// scheme instead.
| VALKEY_URL: str = "valkey://localhost:6379/0" | |
| REDIS_URL: str = "valkey://localhost:6379/0" | |
| CELERY_BROKER_URL: str = "valkey://localhost:6379/0" | |
| CELERY_RESULT_BACKEND: str = "valkey://localhost:6379/0" | |
| VALKEY_URL: str = "redis://localhost:6379/0" | |
| REDIS_URL: str = "redis://localhost:6379/0" | |
| CELERY_BROKER_URL: str = "redis://localhost:6379/0" | |
| CELERY_RESULT_BACKEND: str = "redis://localhost:6379/0" |
| - name: worker | ||
| image: gridify:latest | ||
| imagePullPolicy: Always | ||
| command: ["python", "-m", "temporalio.worker"] |
There was a problem hiding this comment.
The temporalio Python SDK does not provide a built-in runnable module temporalio.worker that can be executed directly from the command line. Running this command will fail. You should create a custom Python script (e.g., run_worker.py) that registers your workflows/activities and starts the worker, and execute that script here.
- name: worker
image: gridify:latest
imagePullPolicy: Always
command: ["python", "backend/run_temporal_worker.py"]| async ready(): Promise<boolean> { | ||
| if (!this.modelUrl) return false; | ||
| if (this.session) return true; | ||
| if (this.worker) return true; | ||
| if (!this.loading) { | ||
| this.loading = this._load(); | ||
| this.loading = this._loadWorker(); | ||
| } | ||
| try { | ||
| await this.loading; | ||
| } catch { | ||
| this.loading = null; | ||
| this.worker = null; | ||
| return false; | ||
| } | ||
| return this.session !== null; | ||
| return this.worker !== null; | ||
| } |
There was a problem hiding this comment.
There is a race condition in ready(). this.worker is assigned immediately at the start of _loadWorker(). If ready() is called again while this.loading is still pending, it will see this.worker is truthy and return true immediately, even though the worker hasn't finished initializing. Furthermore, embed() checks if (!this.worker) and will attempt to post messages to an uninitialized worker, causing runtime errors. Use a separate isReady boolean flag that is only set to true when the ready message is received from the worker.
| const tensor = { | ||
| data: Float32Array.from(vec), | ||
| dims: [1, vec.length], | ||
| type: "float32" as const, | ||
| } as any; |
There was a problem hiding this comment.
Passing a plain object as a tensor to session.run will fail at runtime because onnxruntime-web expects actual ort.Tensor instances. Use the ort.Tensor constructor to instantiate the tensor correctly.
const tensor = new ort.Tensor("float32", Float32Array.from(vec), [1, vec.length]);| endpoint = _flight.FlightEndpoint( | ||
| command, | ||
| [_flight.Location.for_grpc_tcp("0.0.0.0", 8815)], | ||
| ) |
There was a problem hiding this comment.
Using 0.0.0.0 as the location in FlightEndpoint is problematic because 0.0.0.0 is a non-routable address. When a remote client calls get_flight_info, it will receive this address and fail to connect. The location should be a routable hostname or IP address, or at least be configurable.
1 participant
Summary by CodeRabbit
New Features
Performance
Infrastructure