Conversation
![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 adds comprehensive support for GraphQL and WebSockets, including new editors, schema exploration, and background connection management. It also introduces a generic URL fetching utility and migrates macOS vibrancy to a new plugin. The review identifies several critical issues, such as the use of incorrect virtualization APIs and missing dependencies for the WebSocket message list, and the omission of authentication headers during GraphQL introspection. Feedback also highlights that WebSocket SSL verification is hardcoded and inconsistent with its documentation, and suggests improving error handling for GraphQL variable parsing to provide better user feedback.
| import { | ||
| List, | ||
| useListRef, | ||
| useDynamicRowHeight, | ||
| type RowComponentProps, | ||
| } from "react-window"; |
There was a problem hiding this comment.
The virtualization components and hooks (List, useListRef, useDynamicRowHeight) are being imported from react-window, but these are not part of the standard react-window API. Additionally, react-window is not listed in the package.json dependencies. This will lead to a build failure or a runtime crash. Please verify the intended library (e.g., virtua or react-virtuoso) and ensure it is added to the project dependencies.
| async (targetUrl?: string) => { | ||
| const urlToFetch = targetUrl || gql.url; | ||
| if (!urlToFetch) return; | ||
| onStartLoading?.(gql.id); | ||
| setSchemaLoading(true); | ||
| setSchemaError(null); | ||
|
|
||
| try { | ||
| const headers: Record<string, string> = {}; | ||
|
|
||
| (gql.headerItems || []) | ||
| .filter((h) => h.enabled && h.key) | ||
| .forEach((h) => { | ||
| headers[h.key] = h.value; | ||
| }); | ||
|
|
||
| const result = await commands.graphqlIntrospect({ | ||
| url: urlToFetch, | ||
| headers, | ||
| }); | ||
|
|
||
| if (result.status === "error") { | ||
| throw new Error(result.error); | ||
| } | ||
|
|
||
| const { schema_json, error } = result.data; | ||
|
|
||
| if (error) { | ||
| throw new Error(error); | ||
| } | ||
|
|
||
| if (!schema_json) { | ||
| throw new Error("No schema returned from server"); | ||
| } | ||
|
|
||
| const introspectionData = JSON.parse(schema_json) as IntrospectionQuery; | ||
| const schemaObj = buildClientSchema(introspectionData); | ||
| const sdl = printSchema(schemaObj); | ||
|
|
||
| fetchedUrlRef.current = urlToFetch; | ||
| onUpdate((prev) => ({ | ||
| ...prev, | ||
| schemaJSON: schema_json, | ||
| schema: sdl, | ||
| schemaLastFetched: Date.now(), | ||
| })); | ||
| } catch (err: any) { | ||
| setSchemaError(err.message || "Failed to fetch schema"); | ||
| } finally { | ||
| setSchemaLoading(false); | ||
| onStopLoading?.(); | ||
| } |
There was a problem hiding this comment.
The fetchSchema function for GraphQL introspection only includes headers from gql.headerItems and ignores the configured authentication (gql.auth or inherited project auth). Since many GraphQL endpoints require authentication for introspection queries, this will cause schema fetching to fail on protected APIs. You should update this function to compute and include the effective authentication headers, similar to the logic used in handleSendGraphQL.
| let tls = native_tls::TlsConnector::builder() | ||
| .danger_accept_invalid_certs(false) | ||
| .build() | ||
| .map_err(|e| format!("TLS setup error: {e}"))?; |
There was a problem hiding this comment.
The SSL certificate verification is hardcoded to false (strict verification), which contradicts the comment stating it should "Accept any cert in dev". This prevents connections to local or development WebSocket servers using self-signed certificates. This setting should be configurable via the WsConnectRequest to match the flexibility provided for REST requests.
| pub struct WsConnectRequest { | ||
| /// Unique connection ID chosen by the caller (e.g. the WebSocketFile id). | ||
| pub connection_id: String, | ||
| /// The WebSocket URL to connect to (ws:// or wss://). | ||
| pub url: String, | ||
| /// Optional extra headers to include in the upgrade handshake. | ||
| pub headers: HashMap<String, String>, | ||
| /// Optional sub-protocols. | ||
| pub protocols: Vec<String>, | ||
| } |
There was a problem hiding this comment.
The WsConnectRequest struct is missing a verify_ssl field. Adding this field is necessary to allow users to toggle SSL verification for WebSocket connections, which is a common requirement when working with development servers or self-signed certificates.
| let variables: Record<string, unknown> = {}; | ||
| try { | ||
| if (activeGraphQL.variables && activeGraphQL.variables.trim() !== "") { | ||
| variables = JSON.parse(activeGraphQL.variables); | ||
| } | ||
| } catch { | ||
| // invalid JSON variables, send as-is | ||
| } |
There was a problem hiding this comment.
When parsing GraphQL variables, if JSON.parse fails, the error is silently caught and the request proceeds with an empty variables object. This can be misleading for users as their input is ignored without any indication of a syntax error. It would be better to validate the JSON and provide feedback (e.g., via a toast) or prevent the request if the variables are malformed.
1 participant
What's new
Screenshots