Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a comprehensive API management and observability feature set to the project. It adds new RESTful API endpoints for generating SQL, running SQL, generating Vega charts, and streaming explanations, all documented in a newly created OpenAPI specification. Every API call is now logged in a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Route
participant Repository
participant Database
Client->>API_Route: Sends API request (e.g., generate_sql)
API_Route->>Repository: Log request to api_history (start)
API_Route->>API_Route: Process/generate response (AI, SQL, etc.)
API_Route->>Repository: Log response to api_history (complete)
API_Route->>Client: Return API response
Client->>GraphQL_Server: Query apiHistory (with filters/pagination)
GraphQL_Server->>Repository: Fetch history from api_history
Repository->>Database: Query with filters/pagination
Database-->>Repository: Return records
Repository-->>GraphQL_Server: Return API history
GraphQL_Server-->>Client: Return paginated API history
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
wren-ui/src/apollo/server/resolvers/apiHistoryResolver.ts (2)
68-70: Guard against filtering out legitimate 0‑status responses
statusCode0 is falsy but might legitimately occur (e.g. network error logged as 0).
Usetypeof filter.statusCode === 'number'rather than truthiness to avoid skipping it.- if (filter.statusCode) { + if (typeof filter.statusCode === 'number') { filterCriteria.statusCode = filter.statusCode; }
82-88: Validate date strings before constructingDateobjectsAn invalid date (e.g. malformed ISO) silently becomes
Invalid Date, which later breaks SQL where‑clauses or repository look‑ups.- if (filter?.startDate) { - dateFilter.startDate = new Date(filter.startDate); - } + if (filter?.startDate && !Number.isNaN(Date.parse(filter.startDate))) { + dateFilter.startDate = new Date(filter.startDate); + }Apply the same check for
endDate.wren-ui/src/apollo/server/utils/apiUtils.ts (2)
30-69: Consider making startTime parameter optional in respondWith functionThe function declaration requires
startTimeas a non-optional parameter (line 47), but the function body uses a conditional checkstartTime ? Date.now() - startTime : undefinedon line 51. This suggests thatstartTimemight be optional in some usage scenarios.- startTime: number; + startTime?: number;
74-122: Revisit default project ID handling in error scenariosWhen handling errors, if
projectIdis missing, it defaults to0(line 115). Consider if this is the correct approach - using0as a fallback might lead to confusing analytics if this isn't a valid project ID.You might want to consider one of these alternatives:
- Use a special sentinel value like
-1to indicate "no project"- Make the
projectIdtruly optional in the database schema- Log an additional warning when falling back to
0- projectId: projectId || 0, + projectId: projectId ?? -1, // Special value to indicate missing projectwren-ui/src/pages/api/v1/run_sql.ts (1)
125-132: Improve error message specificity for SQL execution failuresThe error message from SQL execution is generic. Consider categorizing errors by type and providing more specific messages to help users troubleshoot issues.
logger.error('Error executing SQL:', queryError); + + // Provide more specific error messages based on error type + let errorMessage = queryError.message || 'Error executing SQL query'; + let errorCode = Errors.GeneralErrorCodes.INVALID_SQL_ERROR; + + // Check for common error patterns to provide better feedback + if (errorMessage.includes('syntax error')) { + errorMessage = 'SQL syntax error: ' + errorMessage; + } else if (errorMessage.includes('permission denied')) { + errorMessage = 'Permission denied: ' + errorMessage; + errorCode = Errors.GeneralErrorCodes.PERMISSION_DENIED; + } throw new ApiError( - queryError.message || 'Error executing SQL query', + errorMessage, 400, - Errors.GeneralErrorCodes.INVALID_SQL_ERROR, + errorCode, );wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (2)
146-148: Consider using a library function for camelToSnakeCaseThe current implementation is simple but might not handle all edge cases (like acronyms). Consider using a library function or a more robust implementation.
private camelToSnakeCase(str: string): string { - return str.replace(/[A-Z]/g, (letter) => `_${letter.toLowerCase()}`); + // Handle special cases like acronyms (e.g., APIType -> api_type not a_p_i_type) + return str + .replace(/([A-Z]+)([A-Z][a-z])/g, '$1_$2') // Handle acronyms followed by capital letter + .replace(/([a-z])([A-Z])/g, '$1_$2') // Standard camelCase conversion + .toLowerCase(); }Alternatively, consider using lodash's
_.snakeCasemethod since you're already importing lodash.
59-81: Add type annotation for query result in count methodThe count method parses the result from a string to a number. Adding a type annotation would make the code clearer and help catch potential issues.
public async count( filter?: Partial<ApiHistory>, dateFilter?: { startDate?: Date; endDate?: Date }, ): Promise<number> { let query = this.knex(this.tableName).count('id as count'); if (filter) { query = query.where(this.transformToDBData(filter)); } if (dateFilter) { if (dateFilter.startDate) { query = query.where('created_at', '>=', dateFilter.startDate); } if (dateFilter.endDate) { query = query.where('created_at', '<=', dateFilter.endDate); } } - const result = await query; + const result: { count: string }[] = await query; return parseInt(result[0].count as string, 10); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
wren-ui/migrations/20250511000000-create-api-history.js(1 hunks)wren-ui/package.json(1 hunks)wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts(1 hunks)wren-ui/src/apollo/server/repositories/index.ts(1 hunks)wren-ui/src/apollo/server/resolvers.ts(4 hunks)wren-ui/src/apollo/server/resolvers/apiHistoryResolver.ts(1 hunks)wren-ui/src/apollo/server/schema.ts(2 hunks)wren-ui/src/apollo/server/types/context.ts(2 hunks)wren-ui/src/apollo/server/utils/apiUtils.ts(1 hunks)wren-ui/src/apollo/server/utils/error.ts(3 hunks)wren-ui/src/common.ts(3 hunks)wren-ui/src/pages/api/graphql.ts(2 hunks)wren-ui/src/pages/api/v1/generate_sql.ts(1 hunks)wren-ui/src/pages/api/v1/run_sql.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
wren-ui/src/apollo/server/utils/error.ts (2)
Learnt from: andreashimin
PR: Canner/WrenAI#1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:10.593Z
Learning: The typo in error code enum names ("IDENTIED_AS_GENERAL" and "IDENTIED_AS_MISLEADING_QUERY" instead of "IDENTIFIED_AS_GENERAL" and "IDENTIFIED_AS_MISLEADING_QUERY") in wren-ui/src/apollo/server/utils/error.ts was recognized but intentionally not fixed as it was considered out of scope for PR #1414.
Learnt from: andreashimin
PR: Canner/WrenAI#1414
File: wren-ui/src/apollo/server/utils/error.ts:0-0
Timestamp: 2025-03-18T10:28:26.608Z
Learning: The enum names `IDENTIED_AS_GENERAL` and `IDENTIED_AS_MISLEADING_QUERY` in GeneralErrorCodes enum in wren-ui/src/apollo/server/utils/error.ts have typos (missing "FI" in "IDENTIFIED") but fixing them is not in scope for the current PR.
🧬 Code Graph Analysis (5)
wren-ui/src/apollo/server/types/context.ts (1)
wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (1)
IApiHistoryRepository(30-40)
wren-ui/src/common.ts (2)
wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (1)
ApiHistoryRepository(42-149)wren-ui/tools/knex.js (1)
knex(30-30)
wren-ui/src/apollo/server/resolvers.ts (1)
wren-ui/src/apollo/server/resolvers/apiHistoryResolver.ts (1)
ApiHistoryResolver(41-132)
wren-ui/src/pages/api/v1/generate_sql.ts (5)
wren-ui/src/common.ts (1)
components(211-211)wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (1)
ApiHistory(10-22)wren-ui/src/pages/api/v1/run_sql.ts (1)
handler(51-146)wren-ui/src/apollo/server/utils/apiUtils.ts (3)
ApiError(12-25)respondWith(30-69)handleApiError(74-122)wren-ui/src/apollo/server/models/adaptor.ts (1)
WrenAIError(11-14)
wren-ui/src/apollo/server/resolvers/apiHistoryResolver.ts (2)
wren-ui/src/apollo/server/types/context.ts (1)
IContext(40-80)wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (1)
ApiHistory(10-22)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (24)
wren-ui/src/apollo/server/repositories/index.ts (1)
20-20: LGTM: Export pattern is consistent.The export statement follows the same pattern as the other repository exports, maintaining consistency in the codebase.
wren-ui/src/apollo/server/types/context.ts (2)
21-21: LGTM: Import added correctly.The import of
IApiHistoryRepositoryfollows the existing pattern of repository interface imports.
75-75: LGTM: Repository added to context interface.The
apiHistoryRepositoryproperty is correctly added to theIContextinterface, allowing GraphQL resolvers and other components to access API history data.wren-ui/src/common.ts (3)
20-20: LGTM: Import added correctly.The
ApiHistoryRepositoryimport follows the existing pattern of repository imports.
73-73: LGTM: Repository instance initialized correctly.The
ApiHistoryRepositoryis properly instantiated with the existingknexinstance, following the same pattern as other repositories.
184-185: LGTM: Repository exposed in components object.The
apiHistoryRepositoryis correctly added to the returned components object, making it available throughout the application.wren-ui/src/apollo/server/utils/error.ts (4)
52-55: LGTM: Error codes for new API functionality added correctly.The new error codes
NON_SQL_QUERYandNO_DEPLOYMENT_FOUNDare appropriate for the RESTful API endpoints being implemented and follow the existing pattern.
111-115: LGTM: Error messages for new error codes added correctly.The error messages are clear and informative, matching the style of existing messages in the codebase.
142-144: LGTM: Short messages for new error codes added correctly.The short messages maintain consistency with the longer error messages and provide concise descriptions for UI display.
49-50: Note existing typos in enum names.There are typos in the enum names
IDENTIED_AS_GENERALandIDENTIED_AS_MISLEADING_QUERY(should be "IDENTIFIED"). Based on the retrieved learnings, these were intentionally left unchanged in a previous PR as fixing them was out of scope.If you plan to fix these typos in the future, consider creating a separate issue to track this refactoring task to avoid breaking changes.
wren-ui/src/pages/api/graphql.ts (2)
48-48: LGTM: Added API history repository to components destructuring.The
apiHistoryRepositoryis correctly added to the list of destructured repositories from the components object.
157-157: LGTM: Added API history repository to GraphQL context.The
apiHistoryRepositoryis properly added to the GraphQL context, making it accessible to resolvers.wren-ui/src/apollo/server/resolvers.ts (4)
10-10: LGTM: ApiHistoryResolver import added.The
ApiHistoryResolverimport is correctly added to support the new API history functionality.
22-22: LGTM: ApiHistoryResolver instantiated.The
apiHistoryResolveris properly instantiated following the pattern used for other resolvers.
74-76: LGTM: API History query field added.The
apiHistoryquery field is correctly added to the root Query resolver, pointing to thegetApiHistorymethod on theapiHistoryResolver.
194-196: LGTM: ApiHistoryResponse nested resolver added.The nested resolver for
ApiHistoryResponseis properly added, which will handle field-specific transformations like date formatting and payload sanitization as seen in the relevant code snippets.wren-ui/migrations/20250511000000-create-api-history.js (1)
36-38: LGTM: Down migration correctly drops the table.The down migration appropriately drops the
api_historytable.wren-ui/src/apollo/server/utils/apiUtils.ts (2)
12-25: LGTM: Clean error class implementationThe
ApiErrorclass is well-designed, extending the native Error class while adding HTTP status codes and optional error codes for more detailed error reporting.
1-8: Enhancing API utilities with comprehensive error logging and response formattingThe utility module appropriately imports necessary dependencies and establishes the connection to the API history repository.
wren-ui/src/pages/api/v1/run_sql.ts (3)
34-49: Well-implemented data transformation functionThe
transformToObjectsfunction is well-designed, with proper input validation and efficient transformation of row data into a more usable object format keyed by column names.
72-80: LGTM: Good deployment validation checkThe code properly checks for an existing deployment before executing SQL, providing a clear error message with the appropriate error code when no deployment is found.
51-70: LGTM: Clean request handling and validationThe main handler function cleanly extracts request parameters, initializes timing metrics, and performs appropriate method and input validation.
wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (2)
86-122: LGTM: Well-implemented pagination and filteringThe
findAllWithPaginationmethod is well-structured with clear filtering, date range handling, and pagination logic including proper default sorting.
42-54: LGTM: Well-structured repository classThe
ApiHistoryRepositoryclass is well-designed, extending the base repository and implementing the appropriate interface with clear identification of JSON columns.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (4)
wren-ui/src/pages/api/v1/generate_sql.ts (2)
90-91:⚠️ Potential issueHandle missing deployment to prevent runtime crash
The current implementation doesn't check if
lastDeployexists before accessinglastDeploy.hashon line 99. This can cause a runtime error if there's no deployment for the project yet.- const lastDeploy = await deployService.getLastDeployment(project.id); + const lastDeploy = await deployService.getLastDeployment(project.id); + + if (!lastDeploy) { + throw new ApiError( + 'No deployment found, please deploy your project first', + 400, + Errors.GeneralErrorCodes.NO_DEPLOYMENT_FOUND, + ); + }
109-116:⚠️ Potential issueAdd a timeout/back-off to the polling loop
The current implementation uses an unbounded
while (true)loop which could:
- Block the serverless function indefinitely
- Lead to excessive API calls if the upstream service is slow/unresponsive
- Potentially cause cost issues in a serverless environment
- let result: AskResult; - while (true) { + const MAX_WAIT_MS = 60_000; // 1 min + const POLL_INTERVAL_MS = 1_000; + const deadline = Date.now() + MAX_WAIT_MS; + + let result: AskResult; + while (Date.now() < deadline) { result = await wrenAIAdaptor.getAskResult(task.queryId); if (isAskResultFinished(result)) { break; } - await new Promise((resolve) => setTimeout(resolve, 1000)); // poll every second + await new Promise((r) => setTimeout(r, POLL_INTERVAL_MS)); } + + if (!isAskResultFinished(result)) { + throw new ApiError( + 'AI service timed out while generating SQL', + 504, + ); + }wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (2)
5-9: Document and adopt a scalable pattern for ApiType extensionThe
ApiTypeenum will likely grow as more API endpoints are added. Consider documenting the pattern for extending this enum and updating any switch statements or mappers that depend on it.Choose and document one of these patterns:
String Enum with Extension Guidelines:
Add documentation on how to add new types and update consumers.Union of String Literals:
export type ApiType = | 'GENERATE_SQL' | 'RUN_SQL' | 'GENERATE_VEGA_SPEC' | 'NEW_TYPE'; // Add new values hereReadonly Object with
as const:export const ApiTypes = { GENERATE_SQL: 'GENERATE_SQL', RUN_SQL: 'RUN_SQL', GENERATE_VEGA_SPEC: 'GENERATE_VEGA_SPEC', // NEW_TYPE: 'NEW_TYPE', } as const; export type ApiType = typeof ApiTypes[keyof typeof ApiTypes];
125-142: 🛠️ Refactor suggestionImprove JSON handling in transformFromDBData
The current JSON parsing implementation can throw unhandled exceptions if the input is invalid JSON. Add error handling to make the code more robust.
protected override transformFromDBData = (data: any): ApiHistory => { if (!isPlainObject(data)) { throw new Error('Unexpected dbdata'); } const camelCaseData = mapKeys(data, (_value, key) => camelCase(key)); const formattedData = mapValues(camelCaseData, (value, key) => { if (this.jsonbColumns.includes(key)) { // The value from Sqlite will be string type, while the value from PG is JSON object if (typeof value === 'string') { - return value ? JSON.parse(value) : value; + if (!value) return value; + try { + return JSON.parse(value); + } catch (error) { + console.error(`Failed to parse JSON for ${key}:`, error); + return value; // Return raw value if parsing fails + } } else { return value; } } return value; }) as ApiHistory; return formattedData; };
🧹 Nitpick comments (4)
wren-ui/src/pages/api/v1/stream_explanation.ts (2)
19-26: Emit proper SSE framing for each chunk
res.write(chunk)forwards raw data, but the SSE protocol expects messages of the form:data: <payload>\n\nUnless
wrenAIAdaptoralready frames the stream this way (worth verifying), clients may not understand the stream.- // pass the chunk directly to the client - res.write(chunk); + const payload = + typeof chunk === 'string' ? chunk : chunk.toString('utf8'); + res.write(`data: ${payload}\n\n`);
24-27: Ensure final event follows SSE specFor the completion message you correctly include
data:but you also need the trailing blank line to flush the event. Add\n\n(two line breaks) after the JSON payload; otherwise some clients won’t dispatch the event.- res.write(`data: ${JSON.stringify({ done: true })}\n\n`); + res.write(`data: ${JSON.stringify({ done: true })}\n\n`);(Just removing the extra newline in comment here for clarity; ensure two CRLFs.)
wren-ui/src/pages/api/v1/generate_sql.ts (1)
49-62: Add safety check for null histories in transformHistoryInputWhile line 51 handles the case when
historiesis falsy, it does not verify that the items within the array are not null before accessing their properties, which could lead to runtime errors.const transformHistoryInput = (histories: ApiHistory[]) => { if (!histories) { return []; } return histories .filter( (history) => - history.responsePayload?.sql && history.requestPayload?.question, + history?.responsePayload?.sql && history?.requestPayload?.question, ) .map((history) => ({ question: history.requestPayload?.question, sql: history.responsePayload?.sql, })); };wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (1)
84-123: Consider adding error handling for database queriesThe
findAllWithPaginationmethod doesn't handle potential database errors. Consider wrapping the query execution in a try/catch block to provide more context about database failures.public async findAllWithPagination( filter?: Partial<ApiHistory>, dateFilter?: { startDate?: Date; endDate?: Date }, pagination?: PaginationOptions, ): Promise<ApiHistory[]> { let query = this.knex(this.tableName).select('*'); // ... query building code ... - const result = await query; - return result.map(this.transformFromDBData); + try { + const result = await query; + return result.map(this.transformFromDBData); + } catch (error) { + console.error('Error fetching API history:', error); + throw new Error(`Failed to fetch API history: ${error.message}`); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
wren-ui/openapi.yaml(1 hunks)wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts(1 hunks)wren-ui/src/apollo/server/utils/apiUtils.ts(1 hunks)wren-ui/src/apollo/server/utils/dataUtils.ts(1 hunks)wren-ui/src/apollo/server/utils/error.ts(3 hunks)wren-ui/src/pages/api/v1/generate_sql.ts(1 hunks)wren-ui/src/pages/api/v1/generate_vega_spec.ts(1 hunks)wren-ui/src/pages/api/v1/run_sql.ts(1 hunks)wren-ui/src/pages/api/v1/stream_explanation.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- wren-ui/src/apollo/server/utils/error.ts
- wren-ui/src/pages/api/v1/run_sql.ts
- wren-ui/src/apollo/server/utils/apiUtils.ts
🔇 Additional comments (3)
wren-ui/openapi.yaml (2)
1-9: The OpenAPI specification looks goodThe OpenAPI 3.0 specification is well-structured and provides clear documentation for the API endpoints. The server URL, paths, and operations are properly defined.
192-217: Well-defined schemas for consistencyThe component schemas are well-defined and reused across the specification, promoting consistency in the API responses. This makes the API more predictable and easier to use.
wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (1)
11-23: The ApiHistory interface is well-structuredThe ApiHistory interface provides a complete and well-typed representation of API call records, including all necessary metadata like projectId, apiType, headers, payloads, and timing information.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wren-ui/src/pages/api/v1/generate_vega_spec.ts (1)
58-64:⚠️ Potential issueTighten
sampleSizeupper bound to mitigate DoS risk.
Allowing up to 1 000 000 rows can strain DB, memory and network. Previous review suggested 10 000, which covers most “preview” use-cases.- sampleSize > 1000000 + sampleSize > 10_000Also document this limit in the OpenAPI spec to set caller expectations.
🧹 Nitpick comments (4)
wren-ui/src/utils/vegaSpecUtils.ts (2)
67-69: Remove unintended leading whitespace inaxis.labelFont.
The font family string has a leading space (' Roboto...') which causes the browser to interpret the first entry as an empty font family and then fall back.- labelFont: ' Roboto, Arial, Noto Sans, sans-serif', + labelFont: 'Roboto, Arial, Noto Sans, sans-serif',
248-270: Broaden default-colour inference and guard against emptyparams.
- Only
nominalaxes are checked;ordinalcategories are equally common in bar/line charts.this.params[0]is assumed to exist. If future callers omithoverparams, this line throws.- const nominalAxis = ['x', 'y'].find( - (axis) => this.encoding[axis]?.type === 'nominal', + const categoryAxis = ['x', 'y'].find( + (axis) => + ['nominal', 'ordinal'].includes(this.encoding[axis]?.type as string), ); ... - if (this.params && this.encoding.color?.field) { - this.params[0].select.fields = [this.encoding.color.field]; + if (this.params?.length && this.encoding.color?.field) { + this.params[0].select = { + ...this.params[0].select, + fields: [this.encoding.color.field], + }; }wren-ui/src/utils/vegaSpecUtils.test.ts (1)
144-145: Removeconsole.logfrom test suite.
Extra logging clutters CI output and slows test runs. Use Jest’s snapshot or assertions instead.- console.log(JSON.stringify(enhancedSpec, null, 2));wren-ui/src/pages/api/v1/generate_vega_spec.ts (1)
110-131: Back-off polling to reduce adaptor load.
Constant 1 s polling for up to 3 minutes (180 calls) may overload the adaptor under heavy traffic. Implement exponential back-off or a fixed-but-longer interval after the first few attempts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
wren-ui/migrations/20250511000000-create-api-history.js(1 hunks)wren-ui/package.json(1 hunks)wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts(1 hunks)wren-ui/src/apollo/server/resolvers/apiHistoryResolver.ts(1 hunks)wren-ui/src/apollo/server/schema.ts(2 hunks)wren-ui/src/apollo/server/utils/dataUtils.ts(1 hunks)wren-ui/src/apollo/server/utils/error.ts(3 hunks)wren-ui/src/pages/api/v1/generate_sql.ts(1 hunks)wren-ui/src/pages/api/v1/generate_vega_spec.ts(1 hunks)wren-ui/src/pages/api/v1/stream_explanation.ts(1 hunks)wren-ui/src/utils/vegaSpecUtils.test.ts(1 hunks)wren-ui/src/utils/vegaSpecUtils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- wren-ui/package.json
- wren-ui/src/pages/api/v1/stream_explanation.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- wren-ui/migrations/20250511000000-create-api-history.js
- wren-ui/src/apollo/server/utils/error.ts
- wren-ui/src/apollo/server/utils/dataUtils.ts
- wren-ui/src/pages/api/v1/generate_sql.ts
- wren-ui/src/apollo/server/resolvers/apiHistoryResolver.ts
- wren-ui/src/apollo/server/schema.ts
- wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
wren-ui/src/utils/vegaSpecUtils.test.ts (1)
wren-ui/src/utils/vegaSpecUtils.ts (1)
enhanceVegaSpec(302-308)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
wren-ui/src/utils/vegaSpecUtils.test.ts (1)
146-172: Avoid brittle deep equality on large config objects.
expect(enhancedSpec.config).toEqual(...)will break whenever a non-breaking style tweak lands (e.g., colour palette change). Prefer partial matching withexpect.objectContainingor snapshot specific keys.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
wren-ui/src/pages/api/v1/generate_sql.ts (6)
112-114: Implement flexible language handling for non-enum languagesThe current implementation doesn't use the previous suggestion to handle languages not included in the WrenAILanguage enum. This may cause issues if users provide a valid language code that isn't in the enum.
Based on previous feedback, consider implementing the more flexible approach:
- language: - language || WrenAILanguage[project.language] || WrenAILanguage.EN, + language: language + ? (WrenAILanguage[language] !== undefined + ? WrenAILanguage[language] + : language) // Pass unknown languages through as-is + : (WrenAILanguage[project.language] || WrenAILanguage.EN),
119-135: Consider implementing exponential backoff for pollingWhile you've added a timeout mechanism, continuously polling every second can still be resource-intensive for long-running queries.
Consider implementing an exponential backoff strategy to reduce server load:
const deadline = Date.now() + MAX_WAIT_TIME; + let pollInterval = 1000; // Start with 1 second let result: AskResult; while (true) { result = await wrenAIAdaptor.getAskResult(task.queryId); if (isAskResultFinished(result)) { break; } if (Date.now() > deadline) { throw new ApiError( 'Timeout waiting for SQL generation', 500, Errors.GeneralErrorCodes.POLLING_TIMEOUT, ); } - await new Promise((resolve) => setTimeout(resolve, 1000)); // poll every second + await new Promise((resolve) => setTimeout(resolve, pollInterval)); + // Increase poll interval with a cap (e.g., max 5 seconds) + pollInterval = Math.min(pollInterval * 1.5, 5000); }
203-210: Remove duplicate threadId parameterThere is a redundancy in the
respondWithcall parameters wherethreadIdis passed twice.await respondWith({ res, statusCode: 200, responsePayload: { sql, threadId: newThreadId, }, projectId: project.id, apiType: ApiType.GENERATE_SQL, startTime, requestPayload: req.body, - threadId: newThreadId, + threadId: newThreadId, // Already passed in responsePayload headers: req.headers as Record<string, string>, });
107-115: Better handle missing thread historyWhen
historiesis undefined,transformHistoryInputis called with undefined which could lead to runtime errors.Explicitly pass an empty array when histories is undefined:
const task = await wrenAIAdaptor.ask({ query: question, deployId: lastDeploy.hash, - histories: transformHistoryInput(histories) as any, + histories: transformHistoryInput(histories || []) as any, configurations: { language: language || WrenAILanguage[project.language] || WrenAILanguage.EN, }, });
51-64: Add type safety to transformHistoryInputThe
transformHistoryInputfunction is not properly typed and includesas anyin its usage.Improve type safety:
-const transformHistoryInput = (histories: ApiHistory[]) => { +const transformHistoryInput = (histories: ApiHistory[]): Array<{ question: string; sql: string }> => { if (!histories) { return []; } return histories .filter( (history) => history.responsePayload?.sql && history.requestPayload?.question, ) .map((history) => ({ question: history.requestPayload?.question, sql: history.responsePayload?.sql, })); };Then remove the
as anycast when using this function.
73-75: Add validation for returnSqlDialect parameterThere's no validation for the
returnSqlDialectparameter. While it has a default value offalse, it's good practice to validate input types.- const { - question, - threadId, - language, - returnSqlDialect = false, - } = req.body as GenerateSqlRequest; + const { + question, + threadId, + language, + returnSqlDialect: rawReturnSqlDialect = false, + } = req.body as GenerateSqlRequest; + const returnSqlDialect = typeof rawReturnSqlDialect === 'boolean' ? rawReturnSqlDialect : false;
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
wren-ui/src/pages/api-management/history.tsx (2)
25-39: Enhance error handling for the GraphQL query.The error handling is limited to console logging, which might not be sufficient for a production application. Consider adding a user-facing error state in the UI.
const { data, loading } = useApiHistoryQuery({ fetchPolicy: 'cache-and-network', variables: { pagination: { offset: (currentPage - 1) * PAGE_SIZE, limit: PAGE_SIZE, }, filter: { apiType: filters['apiType']?.[0], statusCode: filters['statusCode']?.[0], threadId: filters['threadId']?.[0], }, }, - onError: (error) => console.error(error), + onError: (error) => { + console.error(error); + // Display error notification or set error state + notification.error({ + message: 'Failed to load API history', + description: error.message, + }); + }, });
157-206: The implementation looks good, but there's an unnecessary Fragment.The Table component implementation is well-structured with appropriate loading states, pagination, and filtering. However, there's an unnecessary Fragment wrapper in the description property.
description={ - <> <div> Here you can view the full history of API calls, including request inputs, responses, and execution details.{' '} <Link className="gray-8 underline mr-2" href="https://docs.getwren.ai/oss/guide/api-access/history" target="_blank" rel="noopener noreferrer" > Learn more. </Link> </div> - </> }🧰 Tools
🪛 Biome (1.9.4)
[error] 167-180: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
wren-ui/src/components/pages/apiManagement/DetailsDrawer.tsx (3)
9-11: Add documentation for Props type and use the loading prop.The component defines a loading prop but doesn't use it in the implementation.
type Props = DrawerAction<ApiHistoryResponse> & { loading?: boolean; }; export default function DetailsDrawer(props: Props) { - const { visible, onClose, defaultValue } = props; + const { visible, onClose, defaultValue, loading } = props; // Add loading state handling to the Drawer component
69-78: Add null/undefined check for duration.The component might receive null or undefined for durationMs, which could lead to displaying "undefined ms" or "null ms".
<Typography.Text className="d-block gray-7 mb-2"> Duration </Typography.Text> -<div>{durationMs} ms</div> +<div>{durationMs != null ? `${durationMs} ms` : '-'}</div>
80-87: Add null/undefined check for statusCode.The getStatusTag function is called without checking if statusCode exists, which could lead to an error if statusCode is null or undefined.
<Typography.Text className="d-block gray-7 mb-2"> Status code </Typography.Text> -<div>{getStatusTag(statusCode)}</div> +<div>{statusCode != null ? getStatusTag(statusCode) : '-'}</div>wren-ui/src/utils/table.tsx (2)
7-38: Consider adding more specific TypeScript types.The utility functions have some generic type annotations, but could benefit from more specific typing, especially for the filter-related parameters.
-export const getColumnSearchProps = (props: { +export const getColumnSearchProps = <T extends Record<string, any>>(props: { dataIndex: string; placeholder?: string; - onFilter?: (value: string, record: any) => boolean; + onFilter?: (value: string, record: T) => boolean; filteredValue?: any[]; }) => ({ // ... }); -export const getColumnDateFilterProps = (props: { +export const getColumnDateFilterProps = <T extends Record<string, any>>(props: { dataIndex: string; - onFilter?: (value: any, record: any) => boolean; + onFilter?: (value: [string, string], record: T) => boolean; filteredValue?: [string, string] | null; }) => ({ // ... });
105-110: Add safer handling of null/undefined dates.The date formatting code doesn't properly handle all edge cases with null or undefined date values.
onChange={(dates) => { const values = dates - ? [dates[0]?.format('YYYY-MM-DD'), dates[1]?.format('YYYY-MM-DD')] + ? [ + dates[0] ? dates[0].format('YYYY-MM-DD') : null, + dates[1] ? dates[1].format('YYYY-MM-DD') : null + ].filter(Boolean) : []; setSelectedKeys(values); }}wren-ui/src/components/code/BaseCodeBlock.tsx (3)
96-105: Consider using a more React-friendly approach for CSS injectionThe current implementation directly manipulates the DOM to inject CSS, which is generally discouraged in React applications. Consider using styled-components' GlobalStyle component or another CSS-in-JS approach that's more aligned with React's lifecycle.
-export const addThemeStyleManually = (cssText: string) => { - const id = 'ace-tomorrow'; - const themeElement = document.getElementById(id); - if (!themeElement) { - const styleElement = document.createElement('style'); - styleElement.id = id; - document.head.appendChild(styleElement); - styleElement.appendChild(document.createTextNode(cssText)); - } -}; +import { createGlobalStyle } from 'styled-components'; + +export const createThemeStyle = (cssText: string) => { + const ThemeStyle = createGlobalStyle`${cssText}`; + return ThemeStyle; +};Then in your component:
// Inside the CodeBlock component const ThemeStyle = useMemo(() => createThemeStyle(cssText), [cssText]); // In the return statement return ( <> <ThemeStyle /> <Block>...</Block> </> );
107-127: Consider memoizing tokenizer initialization for performanceThe tokenizer and highlighting rules are recreated on each render, which could impact performance for large code blocks. Consider using
useMemoto optimize this.export const createCodeBlock = (HighlightRules: any) => { return function CodeBlock(props: BaseProps) { const { code, copyable, maxHeight, inline, loading, showLineNumbers, backgroundColor, } = props; const { ace } = window as any; - const { Tokenizer } = ace.require('ace/tokenizer'); - const rules = new HighlightRules(); - const tokenizer = new Tokenizer(rules.getRules()); + + const tokenizer = useMemo(() => { + const { Tokenizer } = ace.require('ace/tokenizer'); + const rules = new HighlightRules(); + return new Tokenizer(rules.getRules()); + }, [ace]); useEffect(() => { const { cssText } = ace.require('ace/theme/tomorrow'); addThemeStyleManually(cssText); }, []);
128-161: Consider memoizing tokenized lines for performanceThe tokenization of lines happens on every render. For large code blocks, this could be inefficient. Consider using
useMemoto cache the tokenized lines based on the code input.- const lines = (code || '').split('\n').map((line, index) => { + const lines = useMemo(() => { + return (code || '').split('\n').map((line, index) => { const tokens = tokenizer.getLineTokens(line).tokens; const children = tokens.map((token, index) => { const classNames = token.type.split('.').map((name) => `ace_${name}`); return ( <span key={index} className={classNames.join(' ')}> {token.value} </span> ); }); return ( <span className="adm-code-line ace_line" key={`${line}-${index}`}> {showLineNumbers && ( <span className="adm-code-line-number">{index + 1}</span> )} {children} </span> ); - }); + }); + }, [code, showLineNumbers, tokenizer]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
wren-ui/src/apollo/client/graphql/__types__.ts(3 hunks)wren-ui/src/apollo/client/graphql/apiManagement.generated.ts(1 hunks)wren-ui/src/apollo/client/graphql/apiManagement.ts(1 hunks)wren-ui/src/components/HeaderBar.tsx(1 hunks)wren-ui/src/components/code/BaseCodeBlock.tsx(1 hunks)wren-ui/src/components/code/JsonCodeBlock.tsx(1 hunks)wren-ui/src/components/code/SQLCodeBlock.tsx(1 hunks)wren-ui/src/components/editor/AceEditor.tsx(1 hunks)wren-ui/src/components/editor/CodeBlock.tsx(0 hunks)wren-ui/src/components/layouts/PageLayout.tsx(1 hunks)wren-ui/src/components/modals/SaveAsViewModal.tsx(2 hunks)wren-ui/src/components/pages/apiManagement/DetailsDrawer.tsx(1 hunks)wren-ui/src/components/pages/home/promptThread/ViewSQLTabContent.tsx(2 hunks)wren-ui/src/components/pages/knowledge/SQLPairDrawer.tsx(2 hunks)wren-ui/src/components/pages/knowledge/utils.tsx(0 hunks)wren-ui/src/components/pages/modeling/metadata/ViewMetadata.tsx(2 hunks)wren-ui/src/components/sidebar/APIManagement.tsx(1 hunks)wren-ui/src/components/sidebar/Knowledge.tsx(4 hunks)wren-ui/src/components/sidebar/index.tsx(2 hunks)wren-ui/src/components/table/BaseTable.tsx(2 hunks)wren-ui/src/pages/api-management/history.tsx(1 hunks)wren-ui/src/pages/knowledge/instructions.tsx(4 hunks)wren-ui/src/pages/knowledge/question-sql-pairs.tsx(5 hunks)wren-ui/src/utils/enum/index.ts(1 hunks)wren-ui/src/utils/enum/knowledge.ts(0 hunks)wren-ui/src/utils/enum/menu.ts(1 hunks)wren-ui/src/utils/enum/path.ts(1 hunks)wren-ui/src/utils/icons.ts(2 hunks)wren-ui/src/utils/table.tsx(1 hunks)wren-ui/src/utils/time.ts(1 hunks)
💤 Files with no reviewable changes (3)
- wren-ui/src/components/pages/knowledge/utils.tsx
- wren-ui/src/utils/enum/knowledge.ts
- wren-ui/src/components/editor/CodeBlock.tsx
✅ Files skipped from review due to trivial changes (13)
- wren-ui/src/components/editor/AceEditor.tsx
- wren-ui/src/components/pages/knowledge/SQLPairDrawer.tsx
- wren-ui/src/components/pages/modeling/metadata/ViewMetadata.tsx
- wren-ui/src/components/table/BaseTable.tsx
- wren-ui/src/utils/icons.ts
- wren-ui/src/utils/enum/path.ts
- wren-ui/src/components/modals/SaveAsViewModal.tsx
- wren-ui/src/utils/enum/menu.ts
- wren-ui/src/components/sidebar/APIManagement.tsx
- wren-ui/src/utils/enum/index.ts
- wren-ui/src/components/code/SQLCodeBlock.tsx
- wren-ui/src/utils/time.ts
- wren-ui/src/components/layouts/PageLayout.tsx
🧰 Additional context used
🧬 Code Graph Analysis (7)
wren-ui/src/components/sidebar/index.tsx (1)
wren-ui/src/components/sidebar/APIManagement.tsx (1)
APIManagement(27-68)
wren-ui/src/pages/knowledge/question-sql-pairs.tsx (1)
wren-ui/src/components/layouts/PageLayout.tsx (1)
PageLayout(10-26)
wren-ui/src/components/pages/apiManagement/DetailsDrawer.tsx (2)
wren-ui/src/apollo/client/graphql/__types__.ts (1)
ApiHistoryResponse(64-77)wren-ui/src/utils/time.ts (1)
getAbsoluteTime(15-17)
wren-ui/src/components/code/JsonCodeBlock.tsx (1)
wren-ui/src/components/code/BaseCodeBlock.tsx (2)
BaseProps(9-17)createCodeBlock(107-201)
wren-ui/src/pages/knowledge/instructions.tsx (1)
wren-ui/src/components/layouts/PageLayout.tsx (1)
PageLayout(10-26)
wren-ui/src/components/code/BaseCodeBlock.tsx (1)
wren-ui/src/components/PageLoading.tsx (1)
Loading(70-79)
wren-ui/src/apollo/client/graphql/apiManagement.generated.ts (1)
wren-ui/src/apollo/client/graphql/__types__.ts (4)
Exact(4-4)InputMaybe(3-3)ApiHistoryFilterInput(43-50)ApiHistoryPaginationInput(59-62)
🪛 Biome (1.9.4)
wren-ui/src/pages/api-management/history.tsx
[error] 167-180: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (31)
wren-ui/src/components/HeaderBar.tsx (1)
74-81: LGTM - API navigation button added correctlyThe API navigation button follows the same styling and behavior pattern as existing navigation buttons. It correctly highlights when the pathname starts with the API management path and navigates to the API management history page on click.
wren-ui/src/components/pages/home/promptThread/ViewSQLTabContent.tsx (2)
18-20: LGTM - Updated to use specialized SQLCodeBlockThe import has been correctly changed to use the specialized
SQLCodeBlockcomponent for better SQL syntax highlighting.
129-135: LGTM - Component usage correctly updatedThe component reference has been properly updated from
CodeBlocktoSQLCodeBlockwhile maintaining all the same props and functionality.wren-ui/src/components/sidebar/index.tsx (2)
11-11: LGTM - APIManagement component importedThe APIManagement component is correctly imported to be used in the sidebar navigation.
67-69: LGTM - APIManagement integrationThe APIManagement component is correctly integrated into the sidebar with appropriate conditional rendering based on the current pathname. This follows the same pattern as other sidebar components.
wren-ui/src/apollo/client/graphql/apiManagement.ts (1)
3-26: Well-structured GraphQL query for API historyThis GraphQL query is cleanly implemented with proper pagination support and comprehensive field selection. It provides a good foundation for the API history tracking feature.
wren-ui/src/pages/knowledge/instructions.tsx (4)
12-12: Good addition of PageLayout componentThe PageLayout component will help create consistent layouts across pages.
32-32: Appropriate component destructuringClean extraction of Typography components for better readability.
170-230: Improved page structure with PageLayoutThe refactoring to use PageLayout provides better organization and consistency with other pages. The component's props are used correctly to structure the title, action button, and description.
200-200: Consistent table stylingThe addition of the
ant-table-has-headerclass ensures visual consistency with other tables in the application.wren-ui/src/components/sidebar/Knowledge.tsx (3)
5-5: Updated imports for menu systemGood refactoring to use the centralized menu key system.
20-23: Well-structured menu key mappingClean implementation of the mapping between routes and menu keys, making the relationship explicit and maintainable.
39-39: Updated menu item keysConsistent update of menu item keys to use the new enum system.
Also applies to: 50-50
wren-ui/src/pages/knowledge/question-sql-pairs.tsx (6)
6-6: Good addition of PageLayout componentThe PageLayout component will help create consistent layouts across pages.
24-26: Improved code display with specialized componentReplacing the generic CodeBlock with SQLCodeBlock will provide better syntax highlighting specific to SQL, improving readability.
28-28: Appropriate component destructuringClean extraction of Typography components for better readability.
104-104: Enhanced SQL renderingThe SQLCodeBlock provides better SQL syntax highlighting than the generic CodeBlock.
129-192: Improved page structure with PageLayoutThe refactoring to use PageLayout provides better organization and consistency with other pages. The component's props are used correctly to structure the title, action button, and description.
162-162: Consistent table stylingThe addition of the
ant-table-has-headerclass ensures visual consistency with other tables in the application.wren-ui/src/apollo/client/graphql/__types__.ts (2)
43-83: Well-designed API history types and enum.The GraphQL types for API history are comprehensive and follow best practices. The
ApiHistoryFilterInputprovides flexible filtering options,ApiHistoryPaginatedResponseincludes proper pagination metadata, andApiHistoryResponsecaptures all necessary fields for API call history records.
1057-1094: Clean GraphQL schema extension for API history query.The addition of the
apiHistoryfield to the root Query type with optional filtering and pagination is well-designed and consistent with GraphQL best practices.wren-ui/src/components/code/BaseCodeBlock.tsx (6)
1-8: LGTM: Good organization of importsThe imports are well-organized, grouping React, styled-components, Ant Design components, and local imports appropriately.
9-17: LGTM: Well-defined interface with appropriate propsThe
BasePropsinterface is clearly defined with appropriate optional flags and typed properties.
19-36: LGTM: Clean conditional styling approachThe
getBlockStylesfunction provides a clean way to handle inline vs. block styling variants.
38-76: LGTM: Well-structured styled component with comprehensive stylingThe
Blockstyled component has appropriate styling for code blocks, including positioning, fonts, and proper cursor behavior.
78-94: LGTM: Copy button stylingThe
CopyTextcomponent has good positioning and styling for the copy functionality.
162-200: LGTM: Clean component rendering with good accessibility supportThe component rendering is clean with proper loading state handling and keyboard support for selecting code. The copy functionality is well-implemented with visual feedback.
wren-ui/src/apollo/client/graphql/apiManagement.generated.ts (4)
1-5: LGTM: Clean imports and setupThe imports and default options setup follows Apollo Client best practices.
6-12: LGTM: Well-typed query variables and response typesThe TypeScript types for the API History query variables and response are well-defined, ensuring type safety when using the hooks.
15-35: LGTM: Comprehensive GraphQL queryThe GraphQL query is well-structured, requesting all necessary fields for API history, including metadata and pagination information.
37-64: LGTM: Well-documented and typed Apollo hooksThe generated hooks follow Apollo best practices with good JSDoc comments and examples. The types are correctly derived from the query.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (2)
60-82: Consider adding pagination defaults to count method documentation.The count method is well-implemented with filter and date range support, but it would be helpful to clarify in the method's JSDoc comment that without filters it will count all records.
/** * Count API history records with filtering +* @param filter Optional filter to narrow down records +* @param dateFilter Optional date range to limit records by creation date +* @returns Promise resolving to the count of matching records */
153-155: Consider edge case in camelToSnakeCase function.The current implementation assumes the string starts with a lowercase letter. Consider adding handling for strings that might start with an uppercase letter.
private camelToSnakeCase(str: string): string { - return str.replace(/[A-Z]/g, (letter) => `_${letter.toLowerCase()}`); + // Handle strings that might start with an uppercase letter + return str.charAt(0).toLowerCase() + str.slice(1).replace(/[A-Z]/g, (letter) => `_${letter.toLowerCase()}`); }wren-ui/src/pages/api/v1/generate_vega_chart.ts (2)
79-85: Enhance input validation error message for sample size.The current error message "Invalid sampleSize" doesn't provide enough guidance. Consider making it more descriptive by specifying the allowed range.
if ( !Number.isInteger(sampleSize) || sampleSize <= 0 || sampleSize > 1000000 ) { - throw new ApiError('Invalid sampleSize', 400); + throw new ApiError('Invalid sampleSize: must be a positive integer between 1 and 1,000,000', 400); }
160-165: Consider error handling for enhanceVegaSpec.The code calls
enhanceVegaSpecwithout any error handling. If this function throws an exception, it will be caught by the outer try-catch, but with a generic error message. Consider adding specific error handling for this function.// Get the generated Vega spec const vegaSpec = result?.response?.chartSchema; // Enhance the Vega spec with styling and configuration -const enhancedVegaSpec = enhanceVegaSpec(vegaSpec, dataObjects); +let enhancedVegaSpec; +try { + enhancedVegaSpec = enhanceVegaSpec(vegaSpec, dataObjects); +} catch (error) { + throw new ApiError( + `Failed to enhance Vega specification: ${error.message}`, + 500 + ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
wren-ui/openapi.yaml(1 hunks)wren-ui/src/apollo/client/graphql/__types__.ts(3 hunks)wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts(1 hunks)wren-ui/src/apollo/server/resolvers/apiHistoryResolver.ts(1 hunks)wren-ui/src/apollo/server/schema.ts(2 hunks)wren-ui/src/pages/api/v1/generate_sql.ts(1 hunks)wren-ui/src/pages/api/v1/generate_vega_chart.ts(1 hunks)wren-ui/src/pages/api/v1/run_sql.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- wren-ui/src/pages/api/v1/run_sql.ts
- wren-ui/src/apollo/client/graphql/types.ts
- wren-ui/src/apollo/server/schema.ts
- wren-ui/openapi.yaml
- wren-ui/src/pages/api/v1/generate_sql.ts
- wren-ui/src/apollo/server/resolvers/apiHistoryResolver.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
wren-ui/src/apollo/server/repositories/apiHistoryRepository.ts (4)
5-9: API type schema properly defined with support for new chart generation functionality.The enum includes the necessary types: GENERATE_SQL, RUN_SQL, and the new GENERATE_VEGA_CHART, correctly matching the PR objectives.
11-23: Well-structured ApiHistory interface with appropriate field types.The interface properly models API history entries with required and optional fields. The interface uses appropriate types for all properties including JSONB data fields.
47-51: JSONB columns properly identified for special handling.The repository correctly identifies headers, requestPayload, and responsePayload as JSONB columns requiring special processing during data transformation.
133-141: Improved JSON parsing with proper error handling.The code now correctly handles potential JSON parsing errors, addressing a previous review comment. This ensures the application won't crash if it encounters malformed JSON data.
wren-ui/src/pages/api/v1/generate_vega_chart.ts (5)
19-20: Define timeout constant with clear intent.The timeout value is properly set as a constant with a descriptive name, making it easy to understand and modify if needed.
26-40: Comprehensive chart result validation function.The validation function properly checks for error conditions and failed states, throwing appropriate API errors with status codes and error messages.
98-112: Well-handled SQL query execution with appropriate error handling.The code properly wraps the SQL execution in a try-catch block and converts any errors into standardized API errors with appropriate status codes.
167-180: Complete API response with proper project and thread ID tracking.The response is properly structured with the enhanced Vega spec and thread ID. The API history is also correctly logged with all necessary metadata.
181-192: Comprehensive error handling with API history logging.Errors are properly caught and handled using the standardized error handling utility, ensuring that all API calls are logged even in case of failure.
onlyjackfrost
left a comment
onlyjackfrost
left a comment
There was a problem hiding this comment.
LGTM for the BE part
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
wren-ui/src/pages/api-management/history.tsx (6)
25-39: Consider enhancing error handling for the GraphQL query.The error handling currently only logs to the console, which doesn't provide any feedback to the user when errors occur. Consider adding a user-visible error state in the UI to improve the user experience.
const { data, loading, error } = useApiHistoryQuery({ fetchPolicy: 'cache-and-network', variables: { pagination: { offset: (currentPage - 1) * PAGE_SIZE, limit: PAGE_SIZE, }, filter: { apiType: filters['apiType']?.[0], statusCode: filters['statusCode']?.[0], threadId: filters['threadId']?.[0], }, }, - onError: (error) => console.error(error), }); + // Display error message if query fails + if (error) { + return ( + <SiderLayout loading={false} sidebar={null}> + <PageLayout title="API History"> + <Alert + message="Error loading API history" + description={error.message} + type="error" + showIcon + /> + </PageLayout> + </SiderLayout> + ); + }
23-23: Consider using a more specific type for the filters state.The
filtersstate is typed asRecord<string, any>, which is not type-safe. Consider defining a more specific interface for the filters to improve type safety and code maintainability.- const [filters, setFilters] = useState<Record<string, any>>({}); + interface ApiHistoryFilters { + apiType?: string[]; + statusCode?: number[]; + threadId?: string[]; + } + + const [filters, setFilters] = useState<ApiHistoryFilters>({});
167-180: Remove unnecessary Fragment wrapper.The Fragment (
<>...</>) is redundant here since it only contains a single child. Removing it will simplify the code without changing the functionality.description={ - <> <div> Here you can view the full history of API calls, including request inputs, responses, and execution details.{' '} <Link className="gray-8 underline mr-2" href="https://docs.getwren.ai/oss/guide/api-access/history" target="_blank" rel="noopener noreferrer" > Learn more. </Link> </div> - </> }🧰 Tools
🪛 Biome (1.9.4)
[error] 167-180: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
183-200: Consider adding no-data state handling.The table doesn't have explicit handling for when there's no data (empty state). Consider adding a customized empty state message to improve the user experience when there are no API history records to display.
<Table className="ant-table-has-header" dataSource={data?.apiHistory.items || []} loading={loading} columns={columns} rowKey="id" + locale={{ + emptyText: 'No API history records found. Try changing your filters or make some API calls.' + }} pagination={{ hideOnSinglePage: true, pageSize: PAGE_SIZE, size: 'small', total: data?.apiHistory.total, }} scroll={{ x: 1200 }} onChange={(pagination, filters, _sorter) => { setCurrentPage(pagination.current); setFilters(filters); }} />
123-127: Add visual indicator for thread ID search functionality.While the code includes search functionality for the thread ID column, there's no clear visual indicator to users that they can search by thread ID. Consider adding a tooltip or helper text to make this feature more discoverable.
...getColumnSearchProps({ dataIndex: 'threadId', placeholder: 'thread ID', filteredValue: filters['threadId'], + tooltip: 'Search by thread ID', }),
18-39: Consider implementing pagination reset when filters change.Currently, when a user applies a filter, the pagination state isn't reset. This could lead to confusing situations where the user applies a filter that results in fewer pages, but the current page number is still set to a now-invalid page. Consider resetting the current page to 1 when filters change.
// Add a useEffect to reset pagination when filters change + useEffect(() => { + setCurrentPage(1); + }, [filters]); const { data, loading } = useApiHistoryQuery({ fetchPolicy: 'cache-and-network', variables: { pagination: { offset: (currentPage - 1) * PAGE_SIZE, limit: PAGE_SIZE, }, filter: { apiType: filters['apiType']?.[0], statusCode: filters['statusCode']?.[0], threadId: filters['threadId']?.[0], }, }, onError: (error) => console.error(error), });Don't forget to add the import:
- import { useState } from 'react'; + import { useState, useEffect } from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ui/src/components/code/JsonCodeBlock.tsx(1 hunks)wren-ui/src/pages/api-management/history.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-ui/src/components/code/JsonCodeBlock.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/pages/api-management/history.tsx
[error] 167-180: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
wren-ui/src/pages/api-management/history.tsx (2)
1-17: LGTM! Well-structured imports.The imports are organized and include all necessary components for the API History page functionality.
41-155: Well-designed table columns with appropriate renderers and filtering.The table columns are well-structured with appropriate titles, renderers, and filter configurations. The conditional rendering for different types of content (SQL code blocks, status tags, etc.) is implemented correctly.
3 participants
API
generate_sql,run_sql,stream_explanation&generate_vega_specExample
generate_sql
follow-up
run SQL
GraphQL API (API History)
response
Todo
invalid_sqlSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests
Chores