Conversation
WalkthroughThis pull request introduces a new database table ( Changes
Possibly related PRs
Suggested labels
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
wwwy3y3
force-pushed
the
feature/reasoning-api
branch
from
bbc474d to
864e082
Compare
|
@coderabbitai summary |
✅ Actions performedSummary regeneration 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: 1
🔭 Outside diff range comments (2)
wren-ui/src/apollo/server/services/askingService.ts (1)
540-545: 🛠️ Refactor suggestionMissing explicit error handling for
createAskingTask.When
askingTaskTracker.createAskingTaskfails (e.g., DB errors, service unavailability), the code does not catch or handle the exception. Consider adding a try/catch block or error wrapper to provide better error messages and rollback logic as needed.try { const response = await this.askingTaskTracker.createAskingTask({ query: input.question, history, deployId, }); } catch (err) { // handle error more gracefully throw err; }wren-ui/src/apollo/server/services/askingTaskTracker.ts (1)
361-374:⚠️ Potential issueRemove duplicated
isResultChangedmethod.Lines 363-373 repeat the logic from lines 346-359. This appears to be a merge artifact.
361 } 362 - private isResultChanged( - previousResult: AskResult, - newResult: AskResult, - ): boolean { - if (previousResult?.status !== newResult.status) { - return true; - } - return false; - } 374 }
🧹 Nitpick comments (9)
wren-ui/src/apollo/server/schema.ts (2)
587-592: Add caution for potential PII & handle optional fields robustly.These new fields (
rephrasedQuestion,sqlGenerationReasoning,retrievedTables,invalidSql,traceId) help provide context for the asking tasks. However, ensure none of them inadvertently carries user-sensitive data or personally identifiable information. Consider auditing or sanitizing data before storing. Also, confirm each field’s optional nature is acceptable (e.g., verifying that analytics, logs, or other services gracefully handle null/undefined values).
710-710: Ensure null-handling logic for theaskingTaskfield.
askingTaskis introduced as a nullable field. Make sure clients understand that the task may not always be available or that the request for an asking task might fail. Properly handle these scenarios in the UI or API consumers to avoid runtime errors.wren-ui/src/apollo/server/services/askingService.ts (2)
65-65: Add clarifying documentation fortrackedAskingResult.The optional property
trackedAskingResultis convenient for passing the results of an ongoing asking task. Consider adding inline comments or JSDoc to explain how it’s intended to be set, read, and updated. This will help maintain clarity for contributors.
599-605: Consider fallback or error logging in task binding block.When binding the thread response via
askingTaskTracker.bindThreadResponse, ensure that missing or invalid IDs don’t cause hidden failures. For instance, ifinput.trackedAskingResult.queryIdis undefined, the method may fail silently or produce an unclear error. Logging fallback logic or verifying input might help.wren-ui/src/apollo/server/repositories/askingTaskRepository.ts (2)
1-26: Add an index or unique constraint forqueryIdif queries are frequent.This new repository and model definition look solid. Consider indexing
queryIdin theasking_tasktable to speed up frequent lookups and sustain performance. Also verify that concurrency or unique constraints align with howqueryIdis generated.
27-71: Ensure robust validation intransformFromDBDataandtransformToDBData.Converting JSON and handling snake_case/camelCase is done correctly. Be mindful of scenarios where
detailmight be large or have unexpected structure. Properly handle JSON parse failures, e.g., wrapJSON.parsewith try/catch or provide fallback logic to guard against malformed data.wren-ui/src/apollo/server/services/askingTaskTracker.ts (3)
15-16: Consider making the debug logging level configurable.By hardcoding
logger.level = 'debug', production environments may generate excessive logs. Allowing a configurable logging level via environment variables could improve maintainability.
110-115: Use optional chaining for cleaner checks.Instead of performing
if (trackedTask && trackedTask.result) { ... }, you can use optional chaining to simplify your logic.- if (trackedTask && trackedTask.result) { + if (trackedTask?.result) {🧰 Tools
🪛 Biome (1.9.4)
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
168-285: Consider a more robust concurrency approach inpollTasks.This method uses a set to track running jobs and concurrently processes tasks with
Promise.allSettled. For more complex scenarios, you might want to implement prioritized queues or bounded concurrency to avoid potential resource contention or partial results when scaling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
wren-ui/migrations/20250509000000_create_asking_task.js(1 hunks)wren-ui/migrations/20250509000001_add_task_id_to_thread.js(1 hunks)wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts(1 hunks)wren-ui/src/apollo/server/models/adaptor.ts(1 hunks)wren-ui/src/apollo/server/repositories/askingTaskRepository.ts(1 hunks)wren-ui/src/apollo/server/repositories/index.ts(1 hunks)wren-ui/src/apollo/server/repositories/threadResponseRepository.ts(3 hunks)wren-ui/src/apollo/server/resolvers/askingResolver.ts(12 hunks)wren-ui/src/apollo/server/schema.ts(4 hunks)wren-ui/src/apollo/server/services/askingService.ts(15 hunks)wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)wren-ui/src/apollo/server/services/index.ts(1 hunks)wren-ui/src/common.ts(7 hunks)wren-ui/src/apollo/server/resolvers/askingResolver.ts(2 hunks)wren-ui/src/apollo/server/schema.ts(0 hunks)wren-ui/src/apollo/server/services/askingService.ts(2 hunks)wren-ui/src/apollo/server/services/askingTaskTracker.ts(4 hunks)wren-ui/src/common.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (29)
wren-ui/migrations/20250509000001_add_task_id_to_thread.js (1)
1-25: Migration looks well-structuredThis migration properly adds the asking_task_id foreign key to the thread_response table with appropriate nullability and cascade behavior. The down migration also correctly handles the cleanup order by dropping the foreign key first.
The migration filename contains the date "20250509", which appears to be in the future (2025). Please verify if this is intentional for migration sequencing or if it should be adjusted to the current year.
wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts (1)
517-528: The transformation logic looks well-structured.The addition of new properties to the
AskResultobject enhances the response with more detailed information from the AI service. The snake_case to camelCase conversion is applied consistently, and all new fields are properly extracted from the response body.wren-ui/migrations/20250509000000_create_asking_task.js (2)
5-26: Well-structured database table creation for the new asking task tracking feature.The table structure is comprehensive with appropriate constraints:
- Primary key and auto-incrementing ID
- Unique constraint on
query_idto ensure no duplicate tasks- Proper foreign key relationships with cascade deletion to maintain referential integrity
- JSON column for flexible detail storage
- Timestamps for tracking creation and updates
32-34: Migration rollback looks good.The down function properly drops the table when the migration needs to be rolled back.
wren-ui/src/common.ts (6)
17-17: Integration of the new AskingTaskRepository.The import is properly added to the existing list of repositories.
32-32: Integration of the new AskingTaskTracker service.The import is properly added to the existing list of services.
65-65: Repository instantiation looks good.The
askingTaskRepositoryis instantiated with the existingknexinstance, following the pattern used for other repositories.
108-112: Proper service initialization with dependencies.The
AskingTaskTrackeris instantiated with the correct dependencies:
wrenAIAdaptorfor interacting with the AI serviceaskingTaskRepositoryfor persistent storage of asking tasksthreadResponseRepositoryfor relating tasks to thread responsesThis follows the dependency injection pattern used throughout the application.
123-123: Integration with existing AskingService.The
askingTaskTrackeris properly injected into theaskingService, allowing it to leverage the new task tracking functionality.
163-163: Proper export of new components.Both the repository and tracker are exported in the return object, making them accessible to the rest of the application.
Also applies to: 178-178
wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (3)
44-44: Good addition of the foreign key relationship.The optional
askingTaskIdproperty establishes the relationship with the newasking_tasktable, supporting the feature of tracking asking tasks in the background.
48-48: Making SQL field optional is appropriate.Changing the
sqlproperty from required to optional is a good decision as there may be cases where a thread response exists without a corresponding SQL query.
114-139: Update method properly handles optional fields.The
updateOnemethod has been enhanced to:
- Accept optional
sqlandviewIdparameters- Handle these fields properly in the transformation logic, setting them to
undefinedwhen not providedThis aligns well with the changes to the
ThreadResponseinterface and ensures proper handling of optional fields.wren-ui/src/apollo/server/schema.ts (4)
623-623: Validate usage of the newtaskIdfield in CreateThreadInput.Allowing an external
taskIdto be passed in is useful for referencing background tasks. Ensure front-end and other clients provide consistent or valid IDs (e.g., existence in the system, correct format), and handle any conflicts iftaskIdis invalid or previously associated with another thread.
630-630: Consider verifying the newtaskIdin CreateThreadResponseInput.Similar to CreateThreadInput,
taskIdis introduced here. Double-check that incoming requests successfully correlate a valid task to the newly created thread response. Discrepancies intaskIdcould lead to orphaned or incorrectly bound tasks.
705-705: Beware of potential backward incompatibility by changingsqlfrom non-nullable to nullable.Changing
sql: String!tosql: Stringcan break clients expecting a non-null string. Confirm that downstream usage and client-side code handle the possibility ofnullor missing SQL string without error, especially if older consumers rely on the field’s prior non-null guarantee.
889-889: Confirm handling of nullable return value inaskingTaskquery.The schema’s
askingTask(taskId: String!): AskingTaskcan returnAskingTask | null; confirm that consumers properly handle null. Any mismatch could cause confusion or unhandled errors if a non-existent or invalidtaskIdis requested.wren-ui/src/apollo/server/services/askingService.ts (4)
39-39: Review import usage forIAskingTaskTrackerandTrackedAskingResult.This new import correctly references the
IAskingTaskTrackerinterface and theTrackedAskingResulttype. Ensure the source file (./askingTaskTracker) is properly maintained and tested, so that changes to the tracker keep the service stable.
108-109: Validate “getAskingTask” and “getAskingTaskById” usage.The newly introduced methods offer direct retrieval of a tracked task result by either
taskIdor numeric ID. Verify that both methods handle nonexistent or stale tasks gracefully, and confirm no uncaught exceptions occur if the task is already deleted or nonexistent.
367-380: Confirm the lifecycle and injection ofaskingTaskTracker.The new
askingTaskTrackerfield and constructor parameter look correct for dependency injection. Ensure that test environments and mocking scenarios properly instantiate this service, and no memory leaks or mis-scoped references occur during server shutdown.
555-555: Check cancellation edge cases forcancelAskingTask.
this.askingTaskTracker.cancelAskingTask(taskId)may fail if the task is already canceled or never existed, potentially throwing an error. Confirm that these scenarios are properly caught and that the rest of the application can handle an “already canceled” or “not found” outcome without confusion.wren-ui/src/apollo/server/services/askingTaskTracker.ts (1)
63-82: Recheck the polling startup sequence for concurrency or timing issues.Calling
this.startPolling()directly within the constructor might introduce edge cases if other services are not fully initialized or if concurrency is a concern. Confirm that no race conditions arise from early polling.wren-ui/src/apollo/server/resolvers/askingResolver.ts (4)
49-54: Enhanced extensibility acknowledged.These additional optional properties (e.g.,
rephrasedQuestion,sqlGenerationReasoning, etc.) allow for richer context in resultant tasks. This is a clean way to accommodate new data.
195-197: Verify null-return impact on downstream logic.Returning
nullifaskResultis missing may cause consumer code to fail if it expects a valid object. Ensure that clients or calling logic handle this case gracefully.
232-255: Clean handling ofthreadInputbased ontaskId.The logic to retrieve an existing asking task or fallback to direct input is well-structured. This approach avoids duplication and clarifies the data flow for thread creation.
609-639: Transformation logic is well-factored.The
transformAskingTaskmethod cleanly separates data extraction from the subsequent resolver logic, improving readability. Great job structuring the finalAskingTaskobject.wren-ui/src/apollo/server/repositories/index.ts (1)
17-17: Exports added correctlyThe new export for
askingTaskRepositoryfollows the established pattern in this file for making repository functionality available throughout the application. This is a clean addition that will enable tracking of asking tasks as described in the PR objectives.wren-ui/src/apollo/server/services/index.ts (1)
9-9: Exports properly structuredThe export of the new
askingTaskTrackerservice follows the consistent pattern used in this file. This export makes the asking task tracking service available for dependency injection and use by other components, supporting the PR objective of enabling background tracking of asking tasks.wren-ui/src/apollo/server/models/adaptor.ts (1)
124-130: Good type extension with backward compatibilityThe addition of optional properties to the
AskResulttype enhances the reasoning API functionality while maintaining backward compatibility. These new fields provide valuable context about the asking process:
rephrasedQuestion- captures how the system understood the user's questionsqlGenerationReasoning- explains SQL generation logic (pairs well with existingintentReasoning)retrievedTables- shows which tables were considered relevant to the queryinvalidSql- stores any invalid SQL that was generatedtraceId- enables tracing for debugging and monitoringAll properties are correctly marked as optional with the
?modifier, ensuring existing code won't break.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (1)
111-126:getAskingResultmethod
Consider optional chaining to simplify thetrackedTask && trackedTask.resultchecks.- if (trackedTask && trackedTask.result) { + if (trackedTask?.result) {🧰 Tools
🪛 Biome (1.9.4)
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
wren-ui/src/apollo/server/resolvers/askingResolver.ts (1)
609-641:transformAskingTaskmethod
Collecting candidates and retrieving views on-the-fly is convenient. If the number of candidates grows, consider caching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-ui/src/apollo/server/resolvers/askingResolver.ts(11 hunks)wren-ui/src/apollo/server/schema.ts(4 hunks)wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (28)
wren-ui/src/apollo/server/schema.ts (5)
587-593: New fields in theAskingTasktype look good.
These additional properties enhance the information available for each asking task, providing richer debugging and result tracking.
623-623:taskIdaddition toCreateThreadInputis consistent.
AddingtaskIdaligns with the new approach of referencing existing asking tasks.
629-629:taskIdaddition toCreateThreadResponseInputis similarly aligned.
This supports the same design pattern for referencing existing asking tasks in thread responses.
709-709:askingTaskfield inThreadResponse.
This addition properly associates each response with an asking task if present.
888-888:askingTask(taskId: String!)now returnsAskingTask(nullable).
This change aligns with the resolver’s ability to returnnullif the task is not found.wren-ui/src/apollo/server/services/askingTaskTracker.ts (15)
1-18: Imports and logger setup appear correct.
The logger’s debug level ensures detailed polling logs.
19-34:TrackedTaskandTrackedAskingResultinterfaces
They are well-defined for typed result tracking.
35-51:IAskingTaskTrackerinterface
The contract is clearly outlined, matching the tracker’s responsibilities.
53-87: Class fields and constructor
The constructor neatly injects all necessary dependencies and starts polling automatically.
89-109:createAskingTaskmethod
Error handling and logging are robust. Task creation logic looks straightforward.
128-135:getAskingResultByIdmethod
Neatly reusesgetAskingResultby reference toqueryId.
137-139:cancelAskingTaskmethod
Delegated cancellation logic is concise and clear.
141-146:stopPollingmethod
Stops the polling interval cleanly.
147-165:bindThreadResponsemethod
Associating the in-memory tracked task with a DB record is straightforward.
166-171:startPolling
Well-defined approach, scheduling periodic polls viasetInterval.
172-303:pollTasksmethod
Runs concurrent checks withPromise.allSettled. The memory retention cleanup is also handled neatly.
305-330:getAskingResultFromDB
Fetching from the repository if not found in memory is a good fallback approach.
331-353:updateTaskInDatabase
Updates or creates records to persist changes. Logic is clear.
355-361:isTaskFinalized
Checks final states elegantly using a small array of statuses.
363-373:isResultChanged
Currently checks only thestatus. For deeper comparisons, consider further logic if needed.wren-ui/src/apollo/server/resolvers/askingResolver.ts (8)
19-19: ImportingAskingDetailTaskInput.
No issues.
29-29: ImportingTrackedAskingResult.
Ensures typed usage in transformation.
108-108: Bindingthis.transformAskingTask.
Maintaining the correctthiscontext is appropriate here.
196-198: ReturningnullifaskResultis falsy.
This aligns with the updated schema allowing a nullableAskingTask.
230-230:taskIdadded to create thread input.
Allows referencing an existing task when creating a thread.
372-373:taskIdadded to createThreadResponse input.
Same concept extended to thread responses.
385-397: CheckingtaskIdbefore using fallback data.
The error thrown if the task is missing is helpful for debugging.
573-581: Nested resolver forsqlandaskingTask.
Conditionally formatting SQL and fetching the mappedAskingTaskaligns with the schema changes.
|
Note: Change to |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (6)
115-122: Use optional chaining for safer property accessConsider using optional chaining to make the code more concise and safer.
- if (trackedTask && trackedTask.result) { + if (trackedTask?.result) {🧰 Tools
🪛 Biome (1.9.4)
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
223-225: Fix typo in commentThere's a typo in the comment - "MISLEADING_QUER" should be "MISLEADING_QUERY".
- // if it's identified as GENERAL or MISLEADING_QUER + // if it's identified as GENERAL or MISLEADING_QUERY
369-379: Enhanced result change detection logicThe current implementation of
isResultChangedonly checks for status changes, but there could be other important changes like response content or SQL that might be worth detecting.Consider expanding the result change detection to include more fields:
private isResultChanged( previousResult: AskResult, newResult: AskResult, ): boolean { // check status change if (previousResult?.status !== newResult.status) { return true; } + + // check for changes in response content + const prevResponse = previousResult?.response?.[0]; + const newResponse = newResult?.response?.[0]; + + if (prevResponse?.sql !== newResponse?.sql) { + return true; + } + + // Compare other relevant fields + // For example, check if viewId has changed + if (prevResponse?.viewId !== newResponse?.viewId) { + return true; + } return false; }
239-240: Task ID tracking during database updateWhen updating an existing task, you should also update the task ID in memory (similar to what's done for new tasks on line 351).
logger.info(`Updating task ${queryId} in database`); await this.updateTaskInDatabase(queryId, task); + // ensure the task ID is properly tracked in memory + if (this.trackedTasks.get(queryId) && !this.trackedTasks.get(queryId).taskId) { + const taskRecord = await this.askingTaskRepository.findByQueryId(queryId); + if (taskRecord) { + this.trackedTasks.get(queryId).taskId = taskRecord.id; + } + }
250-271: SQL update logic could be more DRYThe conditional logic for updating SQL based on whether there's a viewId or not could be refactored to be more DRY.
- if (response.viewId) { - // get sql from the view - const view = await this.viewRepository.findOneBy({ - id: response.viewId, - }); - await this.threadResponseRepository.updateOne( - task.threadResponseId, - { - sql: view.statement, - viewId: response.viewId, - }, - ); - } else { - await this.threadResponseRepository.updateOne( - task.threadResponseId, - { - sql: response?.sql, - }, - ); - } + const updateData: { sql?: string; viewId?: number } = {}; + + if (response.viewId) { + // get sql from the view + const view = await this.viewRepository.findOneBy({ + id: response.viewId, + }); + updateData.sql = view.statement; + updateData.viewId = response.viewId; + } else if (response?.sql) { + updateData.sql = response.sql; + } + + if (Object.keys(updateData).length > 0) { + await this.threadResponseRepository.updateOne( + task.threadResponseId, + updateData + ); + }
345-353: Add task tracking by ID after creationWhen creating a new task, consider adding it to the
trackedTasksByIdmap as well, to ensure it's accessible via both query ID and task ID.const task = await this.askingTaskRepository.createOne({ queryId, question: trackedTask.question, detail: trackedTask.result, }); // update the task id in memory this.trackedTasks.get(queryId).taskId = task.id; + // track by task ID as well + this.trackedTasksById.set(task.id, this.trackedTasks.get(queryId)); return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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/apollo/server/services/askingTaskTracker.ts (1)
21-21: Consider renaming taskId to pipeline_idAccording to the PR comments,
taskIdshould be changed topipeline_idfor better alignment with the project's standards.Please verify if this change still needs to be implemented throughout the codebase.
…ead/thread response LANGFUSE_SECRET_KEY
wwwy3y3
force-pushed
the
feature/reasoning-api
branch
from
7f5a5d1 to
6c73ee8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (2)
89-109:⚠️ Potential issueMethod signature mismatch in
createAskingTaskimplementation.The implementation doesn't match the interface definition on lines 36-41. The interface defines additional parameters (question, threadId, threadResponseId) that are missing in the implementation.
Apply this diff to fix the method signature:
- public async createAskingTask(input: AskInput): Promise<{ queryId: string }> { + public async createAskingTask( + input: AskInput, + question?: string, + threadId?: number, + threadResponseId?: number, + ): Promise<{ queryId: string }> {
311-335:⚠️ Potential issueInconsistent return type in
getAskingResultFromDB.The method returns
nullwhen no task record is found, but the return type isTrackedAskingResultwithout making it nullable.Apply this diff to fix the return type:
- private async getAskingResultFromDB({ - queryId, - taskId, - }: { - queryId?: string; - taskId?: number; - }): Promise<TrackedAskingResult> { + private async getAskingResultFromDB({ + queryId, + taskId, + }: { + queryId?: string; + taskId?: number; + }): Promise<TrackedAskingResult | null> {
🧹 Nitpick comments (8)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (3)
115-122: Optional chaining opportunity in result handling.The code should use optional chaining to safely access properties of potentially undefined objects.
Apply this diff to use optional chaining:
- if (trackedTask && trackedTask.result) { + if (trackedTask?.result) {🧰 Tools
🪛 Biome (1.9.4)
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
223-232: Typo in comment about AskResultType.There's a typo in the comment: "MISLEADING_QUER" is missing the "Y".
Fix the typo in the comment:
- // if it's identified as GENERAL or MISLEADING_QUER + // if it's identified as GENERAL or MISLEADING_QUERY
172-309: Complex polling logic could benefit from more detailed comments.The
pollTasksmethod contains complex logic for tracking and updating task status. While the code is well-structured, additional comments explaining the decision points and state transitions would improve maintainability.Consider adding more detailed comments to explain:
- The task lifecycle (creation, tracking, finalization)
- When and why tasks are removed from memory
- The criteria for database updates
- How thread response updates are triggered
wren-ui/src/apollo/server/repositories/askingTaskRepository.ts (5)
41-56: Improve error handling and add type safety for transformFromDBDataThe error message "Unexpected dbdata" is not descriptive enough. Consider also adding error handling for JSON.parse which could throw if malformed JSON is encountered.
protected override transformFromDBData = (data: any) => { if (!isPlainObject(data)) { - throw new Error('Unexpected dbdata'); + throw new Error('transformFromDBData: Expected plain object but received different type'); } const camelCaseData = mapKeys(data, (_value, key) => camelCase(key)); const transformData = mapValues(camelCaseData, (value, key) => { if (this.jsonbColumns.includes(key)) { 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 null; + } } return value; } return value; }); return transformData as AskingTask; };
58-70: Improve error handling for transformToDBDataSimilar to the previous comment, enhance the error message for better debugging and consider handling JSON.stringify errors.
protected override transformToDBData = (data: any) => { if (!isPlainObject(data)) { - throw new Error('Unexpected dbdata'); + throw new Error('transformToDBData: Expected plain object but received different type'); } const transformedData = mapValues(data, (value, key) => { if (this.jsonbColumns.includes(key)) { - return JSON.stringify(value); + try { + return JSON.stringify(value); + } catch (error) { + console.error(`Failed to stringify JSON for ${key}:`, error); + return null; + } } else { return value; } }); return mapKeys(transformedData, (_value, key) => snakeCase(key)); };
1-11: Consider adding JSDoc documentationThe code would benefit from JSDoc comments for the interfaces and class to document their purpose, usage, and relationships with other components.
import { Knex } from 'knex'; import { BaseRepository, IBasicRepository } from './baseRepository'; import { camelCase, isPlainObject, mapKeys, mapValues, snakeCase, } from 'lodash'; import { AskResult } from '../models/adaptor'; +/** + * Represents an asking task in the system, tracking the progress of questions + * and their responses through various stages of processing. + */ export interface AskingTask {
23-25: Add documentation for repository interfaceDocument the purpose of this repository interface to make it clear for other developers.
+/** + * Repository interface for managing AskingTask entities, + * providing methods to interact with the asking_task table. + */ export interface IAskingTaskRepository extends IBasicRepository<AskingTask> { findByQueryId(queryId: string): Promise<AskingTask | null>; }
37-39: Consider adding unit tests for this repositoryNew repository classes should have appropriate unit tests to ensure data transformation and query methods work as expected.
Would you like me to suggest a test structure for this repository class?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
wren-ui/migrations/20250509000000_create_asking_task.js(1 hunks)wren-ui/migrations/20250509000001_add_task_id_to_thread.js(1 hunks)wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts(1 hunks)wren-ui/src/apollo/server/models/adaptor.ts(1 hunks)wren-ui/src/apollo/server/repositories/askingTaskRepository.ts(1 hunks)wren-ui/src/apollo/server/repositories/index.ts(1 hunks)wren-ui/src/apollo/server/repositories/threadResponseRepository.ts(3 hunks)wren-ui/src/apollo/server/resolvers/askingResolver.ts(11 hunks)wren-ui/src/apollo/server/schema.ts(4 hunks)wren-ui/src/apollo/server/services/askingService.ts(11 hunks)wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)wren-ui/src/apollo/server/services/index.ts(1 hunks)wren-ui/src/common.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- wren-ui/src/apollo/server/repositories/index.ts
- wren-ui/src/apollo/server/services/index.ts
- wren-ui/migrations/20250509000001_add_task_id_to_thread.js
- wren-ui/src/apollo/server/models/adaptor.ts
- wren-ui/migrations/20250509000000_create_asking_task.js
- wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts
- wren-ui/src/apollo/server/repositories/threadResponseRepository.ts
- wren-ui/src/apollo/server/resolvers/askingResolver.ts
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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/common.ts (7)
18-18: AddedAskingTaskRepositoryto the import statement.Good addition. This repository is needed for the new asking task functionality.
33-33: AddedAskingTaskTrackerto the import statement.Appropriate addition of the service that will handle the tracking of asking tasks.
68-68: Created an instance ofAskingTaskRepository.Good initialization of the repository following the established pattern in this file.
111-116: AddedAskingTaskTrackerinitialization with proper dependencies.The tracker is correctly initialized with all required dependencies: wrenAIAdaptor, askingTaskRepository, threadResponseRepository, and viewRepository. This follows the dependency injection pattern used throughout the codebase.
127-127: AddedaskingTaskTrackeras a parameter to theAskingService.Good integration with the existing service. This allows AskingService to leverage the new task tracking functionality.
171-172: AddedaskingTaskRepositoryto the exported components.Properly exposed the repository for use by other components.
188-188: AddedaskingTaskTrackerto the exported components.Correctly exposed the tracker service so it can be accessed by other components in the application.
wren-ui/src/apollo/server/schema.ts (6)
587-593: Added detailed fields toAskingTasktype to support reasoning pipeline.These additional fields provide more detailed information about the asking task, including reasoning steps and intermediary results. This is a good enhancement that improves the explainability of the AI-generated results.
623-623: ReplacedviewIdwithtaskIdinCreateThreadInput.This change aligns with the PR objective to provide a
taskIdwhen making queries or mutations related to asking tasks. It allows for proper tracking of asking tasks in the background.
629-629: ReplacedviewIdwithtaskIdinCreateThreadResponseInput.Consistent with the change to
CreateThreadInput, this maintains API coherence for task tracking.
704-704: Changedsqlfrom non-nullable to nullable inThreadResponse.Making
sqlnullable allows for scenarios where the thread response might not have SQL yet, such as when an asking task is still in progress. However, ensure all client code properly handles null values.
709-709: AddedaskingTaskfield toThreadResponse.This is a valuable addition that allows clients to access the progress of a task through the thread response object, as mentioned in the PR objectives.
913-913: UpdatedaskingTaskquery return type to be nullable.The change from
AskingTask!toAskingTask(making it nullable) allows this query to handle cases where the task might not exist. This is a good defensive programming practice.wren-ui/src/apollo/server/services/askingService.ts (9)
38-38: ImportIAskingTaskTrackerandTrackedAskingResult.Good addition of the necessary imports for the new task tracking system.
63-63: ReplacedviewIdwithtrackedAskingResultinAskingDetailTaskInput.This change aligns with the PR objectives by moving from view-based references to task-based tracking. The
TrackedAskingResulttype provides more comprehensive information than just an ID.
106-107: UpdatedgetAskingTasksignature and addedgetAskingTaskById.These method signatures now return
TrackedAskingResultwhich provides more comprehensive information about an asking task. The addition ofgetAskingTaskByIdallows retrieving tasks by their database ID in addition to queryId.Handle non-existent tasks gracefully in
getAskingTaskandgetAskingTaskById.While these methods delegate to
this.askingTaskTracker, ensure the service or caller can handle scenarios where the result is null (e.g., the task record was purged, or the ID is invalid). Consider returningnullor throwing a custom error that front-end clients can interpret.
365-365: AddedaskingTaskTrackeras a private field and constructor parameter.Good addition of the new tracker as a dependency, following the established dependency injection pattern in the service.
Also applies to: 377-377, 430-430
536-544: UpdatedcreateAskingTaskto use the task tracker.The implementation now delegates to the task tracker instead of directly calling the AI adaptor. This is a good approach that centralizes task management logic in the tracker class.
550-550: UpdatedcancelAskingTaskto use the task tracker.Similar to the
createAskingTaskchange, this delegates task cancellation to the dedicated tracker.
558-564: ImplementedgetAskingTaskandgetAskingTaskById.Both methods correctly delegate to the task tracker. Remember to handle null responses appropriately at the resolver level.
581-596: Updated thread creation logic to use task tracking.The code now creates a thread response with an asking task ID and then binds the task to the thread response. This supports the new task tracking functionality while maintaining the existing thread creation flow.
637-652: Updated thread response creation to handle task tracking.Similar to thread creation, this correctly binds the task to the thread response after creation.
wren-ui/src/apollo/server/services/askingTaskTracker.ts (7)
19-27: Well-definedTrackedTaskinterface.Good definition of the interface for tracked tasks, including all necessary properties for status tracking and association with thread responses.
29-33: CreatedTrackedAskingResulttype extendingAskResult.This type extension is a good approach, adding task-specific properties to the base AskResult.
35-51: Clear interface definition forIAskingTaskTracker.The interface provides a well-defined contract for the task tracker with all necessary methods for creating, retrieving, canceling, and binding tasks.
53-87: Well-structuredAskingTaskTrackerclass with flexible configuration.The class has a good constructor with sensible defaults for polling intervals and memory retention. All dependencies are properly injected, and the class fields are well-organized.
147-164: Implement thread response binding with proper error handling.Good implementation of the
bindThreadResponsemethod, which includes error checking and updates both in-memory state and the database.
337-359: Efficient database update logic inupdateTaskInDatabase.The method only creates a new record if one doesn't exist, and otherwise updates the existing one. This prevents duplicate entries and maintains data integrity.
247-270: Conditional handling of view-based SQL.The code properly handles both cases where a task result might include a viewId (getting SQL from the view) or direct SQL. This flexibility is important for supporting different types of task results.
wren-ui/src/apollo/server/repositories/askingTaskRepository.ts (2)
12-21: AskingTask interface design looks goodThe AskingTask interface is well-structured with appropriate optional properties and type definitions. The design allows for flexibility in task tracking with optional fields like question, threadId, and threadResponseId.
27-35:Details
❓ Verification inconclusive
Consider PR objectives regarding taskId vs pipeline_id
Note that PR comments mentioned changing
taskIdtopipeline_id. Consider whether this interface and repository should reflect that naming convention.
🏁 Script executed:
#!/bin/bash # Check if there are any references to pipeline_id in the codebase # that might indicate the need for alignment with this repository echo "Searching for pipeline_id references in the codebase:" rg -p "pipeline_id" --type ts echo "\nSearching for taskId references in the codebase:" rg -p "taskId" --type tsLength of output: 4427
Naming Convention Consistency (taskId vs. pipeline_id)
I checked the repository and found that all references—including GraphQL queries, resolvers, services, and client code—usetaskId. There are no occurrences ofpipeline_idanywhere in the codebase.
- If the PR objectives intend to switch to
pipeline_idglobally, then this repository (and all related modules) should be updated accordingly.- If the intent is to remain consistent with the current usage, then keeping
taskIdis appropriate.Please confirm the intended naming convention so that we can ensure consistency across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (1)
357-381:⚠️ Potential issueType mismatch in getAskingResultFromDB return value.
The method returns
nullwhen no task record is found, but the return type isTrackedAskingResultwithout making it nullable.Apply this diff to fix the return type:
- private async getAskingResultFromDB({ - queryId, - taskId, - }: { - queryId?: string; - taskId?: number; - }): Promise<TrackedAskingResult> { + private async getAskingResultFromDB({ + queryId, + taskId, + }: { + queryId?: string; + taskId?: number; + }): Promise<TrackedAskingResult | null> {
🧹 Nitpick comments (4)
wren-ui/src/apollo/server/utils/error.ts (3)
46-47: Fix typo in error code namesThere's a spelling mistake in both error code names: "IDENTIED" should be "IDENTIFIED".
- IDENTIED_AS_GENERAL = 'IDENTIED_AS_GENERAL', - IDENTIED_AS_MISLEADING_QUERY = 'IDENTIED_AS_MISLEADING_QUERY', + IDENTIFIED_AS_GENERAL = 'IDENTIFIED_AS_GENERAL', + IDENTIFIED_AS_MISLEADING_QUERY = 'IDENTIFIED_AS_MISLEADING_QUERY',
97-100: Update error message strings to match the fixed error codesAfter fixing the typo in the error code names, remember to update the corresponding string keys.
- [GeneralErrorCodes.IDENTIED_AS_GENERAL]: - 'The question is identified as a general question, please follow-up ask with more specific questions.', - [GeneralErrorCodes.IDENTIED_AS_MISLEADING_QUERY]: - 'The question is identified as a misleading query, please follow-up ask with more specific questions.', + [GeneralErrorCodes.IDENTIFIED_AS_GENERAL]: + 'The question is identified as a general question, please follow-up ask with more specific questions.', + [GeneralErrorCodes.IDENTIFIED_AS_MISLEADING_QUERY]: + 'The question is identified as a misleading query, please follow-up ask with more specific questions.',
123-125: Update short message strings to match the fixed error codesThese short message keys also need to be updated to match the fixed error codes.
- [GeneralErrorCodes.IDENTIED_AS_GENERAL]: 'Identified as general question', - [GeneralErrorCodes.IDENTIED_AS_MISLEADING_QUERY]: - 'Identified as misleading query', + [GeneralErrorCodes.IDENTIFIED_AS_GENERAL]: 'Identified as general question', + [GeneralErrorCodes.IDENTIFIED_AS_MISLEADING_QUERY]: + 'Identified as misleading query',wren-ui/src/apollo/server/services/askingTaskTracker.ts (1)
421-431: Simplistic isResultChanged implementation.The current
isResultChangedmethod only checks for status changes, but more comprehensive change detection could improve performance by reducing unnecessary updates.Consider extending the change detection to check other important fields:
private isResultChanged( previousResult: AskResult, newResult: AskResult, ): boolean { // Check if either result is null if (!previousResult || !newResult) { return previousResult !== newResult; } // check status change if (previousResult?.status !== newResult.status) { return true; } + // Check for changes in error + if (JSON.stringify(previousResult?.error) !== JSON.stringify(newResult?.error)) { + return true; + } + + // Check for changes in response + if (JSON.stringify(previousResult?.response) !== JSON.stringify(newResult?.response)) { + return true; + } return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
wren-ui/src/apollo/server/resolvers.ts(1 hunks)wren-ui/src/apollo/server/resolvers/askingResolver.ts(12 hunks)wren-ui/src/apollo/server/schema.ts(5 hunks)wren-ui/src/apollo/server/services/askingService.ts(11 hunks)wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)wren-ui/src/apollo/server/telemetry/telemetry.ts(1 hunks)wren-ui/src/apollo/server/utils/error.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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/apollo/server/telemetry/telemetry.ts (1)
54-54: Added new telemetry event for rerunning asking tasksThis addition to the telemetry events aligns well with the new functionality for rerunning asking tasks that's being introduced in this PR.
wren-ui/src/apollo/server/resolvers.ts (1)
94-94: Added new mutation resolver for rerunning asking tasksThe implementation delegates to the AskingResolver as expected, following the established pattern in the codebase.
wren-ui/src/apollo/server/schema.ts (6)
587-593: Enhanced AskingTask type with additional context fieldsThese new fields will provide richer information about the asking task's execution, which will be helpful for debugging and providing better user feedback.
623-623: Replaced viewId with taskId in input typesReplacing
viewIdwithtaskIdaligns with the PR objective of tracking asking tasks in the background. This change reflects a shift from view-based to task-based architecture.Also applies to: 629-629
704-704: Made sql field nullable in ThreadResponseChanging
sql: String!tosql: Stringmakes the field nullable. This has previously been commented on in a review, and it's important to ensure that all client code handles this change appropriately.Ensure all UI components properly handle the nullable
sqlfield in ThreadResponse. Client code that previously assumedsqlwas always present will need to add null checks to prevent runtime errors.#!/bin/bash # Find all places in the codebase where ThreadResponse.sql is accessed directly rg -A 3 -B 3 "\.sql" --type=tsx --type=ts | grep -B 3 -A 3 "threadResponse" | grep -v "sql:"
709-709: Added askingTask field to ThreadResponseThis addition allows clients to access asking task details directly from thread responses, which simplifies the API by reducing the need for separate queries.
992-992: Added rerunAskingTask mutationThis new mutation implements the core functionality described in the PR objectives. It takes a response ID and returns a boolean indicating success.
913-913:Details
❓ Verification inconclusive
Made askingTask query result nullable
Changing the return type from
AskingTask!toAskingTaskmakes it nullable. Ensure that client code handles this potential null value appropriately.
🏁 Script executed:
#!/bin/bash # Find all direct usages of the askingTask query to verify null handling rg -A 5 "askingTask\(" --type=tsx --type=tsLength of output: 73
Revised Verification Request: Verify Nullable Handling for askingTask Query
The initial search command failed due to an unrecognized file type ("tsx"). Please run the updated command below to search across both
.tsand.tsxfiles and confirm that the client code handles the nullable response from theaskingTaskquery appropriately:After verifying the output, ensure that any affected client code is updated to manage the potential
nullvalue.
🏁 Script executed:
#!/bin/bash rg -A 5 "askingTask\(" -g "*.{ts,tsx}"Length of output: 1276
Nullable AskingTask Field: Verify Client-Side Null Handling
The update fromAskingTask!toAskingTaskin the schema now allows anullresponse. Our search shows that client code inwren-ui/src/apollo/client/graphql/home.ts(and its generated counterpart) invokes theaskingTaskquery without any clear null-checks. Please verify that these areas either account for a potentialnullresult (using conditional checks or optional chaining) or update them accordingly to avoid runtime issues.wren-ui/src/apollo/server/services/askingService.ts (9)
38-38: Good addition of the new imports for task tracking.The import of
IAskingTaskTrackerandTrackedAskingResultfrom './askingTaskTracker' supports the new functionality that tracks asking tasks in the background.
61-64: Replaced viewId with trackedAskingResult in AskingDetailTaskInput.The replacement of
viewIdwithtrackedAskingResultaligns with the PR objectives of providing a taskId when making queries or mutations related to asking tasks. This change enables tracking of asking tasks in the background.
104-111: Interface updates to support the new task tracking functionality.Good extension of the
IAskingServiceinterface with:
- A new
rerunAskingTaskmethod for rerunning tasks- Updated
getAskingTaskreturn type toTrackedAskingResult- Added
getAskingTaskByIdmethod to retrieve tasks by IDThese changes properly define the contract for the implementation.
369-369: Properly initialized askingTaskTracker service in constructor.The AskingService now correctly accepts and initializes the askingTaskTracker dependency, which enables the background tracking of asking tasks.
Also applies to: 381-392, 434-434
529-553: Updated createAskingTask to use askingTaskTracker.The method now delegates to the askingTaskTracker service and includes support for rerunning cancelled tasks with new optional parameters. The code correctly handles the necessary deployment ID and history retrieval before delegating to the tracker.
555-584: Well-implemented rerunAskingTask method.This new method allows rerunning an asking task from a previous thread response. It:
- Retrieves the original thread response
- Gets the original question
- Correctly sets up the input and payload
- Calls createAskingTask with the rerunFromCancelled flag
The implementation properly handles error cases and maintains thread relationships.
597-604: Handle non-existent tasks gracefully in getAskingTaskById.Both
getAskingTaskandgetAskingTaskByIdmethods delegate to the tracker without any additional error handling, but should handle cases where tasks don't exist or have been purged.
612-639: Updated createThread to support task tracking.The method has been updated to store the taskId from the tracked asking result and bind the thread response to the task. This enables tracking the relationship between threads and asking tasks.
664-694: Updated createThreadResponse to support task tracking.Similar to the createThread method, this implementation has been updated to bind thread responses to asking tasks when a taskId is provided. The implementation properly handles the relationship between thread responses and asking tasks.
wren-ui/src/apollo/server/services/askingTaskTracker.ts (7)
20-54: Well-defined interfaces and types for the task tracker.The interfaces and types (
TrackedTask,TrackedAskingResult,CreateAskingTaskInput, andIAskingTaskTracker) provide a clear contract for the tracker implementation. The types properly extend existing types with the necessary tracking information.
55-89: Comprehensive class setup with proper dependency injection.The
AskingTaskTrackerclass is well-structured with:
- Clear property definitions
- Proper dependency injection in the constructor
- Default values for polling interval and memory retention time
- Initialization of the polling mechanism
This ensures that the tracker can be easily tested and configured.
91-130: Well-implemented createAskingTask method with error handling.The method:
- Calls the AI service to create a task
- Validates input parameters
- Stores the task in memory
- Handles the special case for rerunning cancelled tasks
- Provides proper error handling and logging
The implementation is robust and correctly handles the various cases.
132-156: Effective implementation of getAskingResult methods.Both
getAskingResultandgetAskingResultByIdmethods check the in-memory cache first and fall back to the database if needed, providing an efficient implementation.🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
168-185: Good implementation of bindThreadResponse.The method properly updates both the in-memory tracking and the database record, maintaining consistency between the two.
193-355: Comprehensive polling mechanism with proper error handling.The
pollTasksmethod:
- Skips already running jobs
- Removes finalized tasks after the retention period
- Updates task status from the AI service
- Updates the database when needed
- Handles task finalization
- Updates thread responses
- Provides thorough error handling and logging
This implementation ensures that tasks are properly tracked and updated.
413-419: Good helper method for checking task finalization.The
isTaskFinalizedmethod clearly defines when a task is considered finalized, making the code more maintainable and easier to understand.wren-ui/src/apollo/server/resolvers/askingResolver.ts (7)
49-56: Enhanced AskingTask interface with additional properties.The AskingTask interface has been extended with additional properties (rephrasedQuestion, sqlGenerationReasoning, retrievedTables, invalidSql, traceId, queryId) to provide more context about the task result. This makes the API more informative and enables better front-end experiences.
196-222: Updated getAskingTask with null handling and transformation.The
getAskingTaskmethod now:
- Properly handles the null case when no task is found
- Uses the new
transformAskingTaskhelper method to convert the result- Maintains the existing telemetry logic
This ensures consistent error handling and data transformation.
227-258: Well-implemented createThread with task ID support.The method has been updated to:
- Accept a taskId instead of a viewId
- Retrieve the asking task result when a taskId is provided
- Use the task's question and result for thread creation
- Fall back to the input data when no taskId is provided
This implementation properly supports the new task tracking functionality.
366-407: Similar good implementation in createThreadResponse.Like createThread, this method has been updated to support taskId and retrieve asking task results. The implementation is consistent with the changes in createThread and properly handles the relationship between thread responses and asking tasks.
420-436: Well-implemented rerunAskingTask method.This new method provides the ability to rerun an asking task from a previous thread response, which is a valuable feature for users. The implementation correctly:
- Retrieves the threadResponseId
- Calls the corresponding service method
- Sends the appropriate telemetry event
- Returns a boolean indicating success
The method is straightforward and focused.
593-600: Added askingTask resolver for thread responses.This allows clients to access the associated asking task from a thread response, providing better visibility into the task's status and results. The implementation correctly handles null cases and transforms the result.
628-661: Comprehensive transformAskingTask helper method.This method transforms the TrackedAskingResult into an AskingTask format expected by the GraphQL API. It:
- Constructs candidates from the response
- Resolves view information for each candidate
- Maps all the required fields including the new ones
- Handles the default type case
The implementation is thorough and properly handles all the necessary transformations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (3)
18-18: Consider making logger level configurable.Currently, the logger is explicitly set to debug mode. This might clutter logs in production. Consider making the logging level configurable so that different environments can set log levels appropriately.
136-136: Prefer optional chaining to simplify the condition.This check can be simplified using optional chaining while preserving equivalent logic:
- if (trackedTask && trackedTask.result) { + if (trackedTask?.result) {🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
187-355: Review concurrency strategy for polling large numbers of tasks.All tasks are polled in parallel every second, which can cause performance bottlenecks as the number of tasks grows. Consider implementing a concurrency limit or a more scalable batch strategy to avoid potential overload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (3)
22-22: RenametaskIdtopipeline_idfor consistency with the updated requirement.Based on the PR objectives, the user indicated that
taskIdshould be changed topipeline_idfor improved clarity. Please confirm the final naming requirement and update both variable names and references accordingly within the codebase.Also applies to: 39-39, 100-100
253-274: Confirm error logic when task is identified as GENERAL or MISLEADING_QUERY.Re-running a canceled task sets its status to FAILED if it falls under GENERAL or MISLEADING_QUERY. Please verify this aligns with your intended business logic, as it might override more granular error statuses or future re-run scenarios.
296-317: Verify multi-candidate handling logic.Currently, only the first candidate is used to update the thread response. If multiple candidates or answers are expected, consider a loop or different selection policy to ensure all relevant results are processed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
wren-ui/src/apollo/server/services/askingService.ts (6)
104-112: Parameter expansion forcreateAskingTaskis coherent.
IntroducingrerunFromCancelled,previousTaskId, andthreadResponseIdfosters clarity for re-running tasks. As the list of optional params grows, consider grouping them into a dedicated options object for cleaner maintainability.
566-597: Implementation ofrerunAskingTask.
- Checks for an existing thread response.
- Reuses the original question.
- Calls
createAskingTaskwith flags for re-running.Potential improvements:
- Add logic to handle partially finished tasks if needed.
- Confirm any concurrency edge cases.
Otherwise, this is a clear approach to re-run.
621-623: Updated doc comments for creating a thread and AI task.
The stepwise breakdown is clearer. Consider revisiting them if future logic changes, but for now, they match the new workflow.
640-649: Binding the new thread response to an asking task if present.
This ensures full linkage. IftaskIdis invalid, consider optionally logging or gracefully ignoring. Right now, it assumes the tracker call always succeeds.
693-693: Applying the sameaskingTaskIdassignment for new thread responses.
This duplicates logic from thread creation, but it’s consistent. If you foresee further expansion, factor out a helper.
696-705: Repeating the bind logic increateThreadResponse.
Identical logic to the previous segment. Consider extracting a helper to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-ui/src/apollo/server/resolvers/askingResolver.ts(13 hunks)wren-ui/src/apollo/server/schema.ts(5 hunks)wren-ui/src/apollo/server/services/askingService.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wren-ui/src/apollo/server/schema.ts
- wren-ui/src/apollo/server/resolvers/askingResolver.ts
🧰 Additional context used
🧠 Learnings (1)
wren-ui/src/apollo/server/services/askingService.ts (2)
Learnt from: wwwy3y3
PR: Canner/WrenAI#1388
File: wren-ui/src/apollo/server/services/askingService.ts:599-604
Timestamp: 2025-03-17T19:26:10.835Z
Learning: Error handling for non-existent asking tasks in WrenAI is handled at the resolver level rather than in the service implementation.
Learnt from: wwwy3y3
PR: Canner/WrenAI#1388
File: wren-ui/src/apollo/server/services/askingService.ts:586-604
Timestamp: 2025-03-17T19:32:09.267Z
Learning: In WrenAI's askingService.ts, errors should be allowed to bubble up to the top-level error handler rather than being caught and logged at the method level.
🧬 Code Definitions (1)
wren-ui/src/apollo/server/services/askingService.ts (4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (2) (2)
TrackedAskingResult(31:35)IAskingTaskTracker(42:53)wren-ui/src/apollo/server/resolvers/askingResolver.ts (1) (1)
Task(38:40)wren-ui/src/apollo/server/repositories/threadRepository.ts (1) (1)
Thread(17:27)wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1) (1)
ThreadResponse(42:52)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (18)
wren-ui/src/apollo/server/services/askingService.ts (18)
28-28: No issues with the additional import.
Looks likeIViewRepositoryandProjectare both referenced within this file, so this import is valid and necessary.
38-38: Importing new tracker interface and result type looks good.
This aligns with the new task-tracking approach introduced in this file.
63-63: Addition oftrackedAskingResultis consistent with new task tracking logic.
Optional property is well-defined, but consider ensuring any thread calling code handles a missingtrackedAskingResultcase gracefully.
113-115: NewrerunAskingTaskmethod signature.
Adding a direct API to re-run a canceled or completed task is a solid approach. Ensure callers handle potential concurrency issues if the original task is still in progress.
118-119: New methods for retrieving asking tasks by string ID or numeric ID.
This split is understandable if the system creates tasks using different key formats. Confirm all calls properly match the correct method based on the observed ID format.
377-377: Introducing a dedicated property foraskingTaskTracker.
Well-structured usage of the interface. No issues spotted.
389-389: Constructor injection of the tracker is consistent with service design.
No concerns here; this aligns with the overall dependency injection pattern.
400-400: Type annotation foraskingTaskTrackeris correct.
This ensures the class property remains strongly typed and consistent with the new interface.
442-442: Assigning the tracker reference is straightforward.
No issues; the property is set properly for later usage.
540-542: Optional parameters increateAskingTaskimplementation.
The function signature now closely matches the interface definition. Make sure to handle scenarios where these optional parameters are undefined.
550-552: Conditionally fetching asking history based onthreadResponseId.
This effectively excludes canceled responses from the conversation flow. Confirm thatthreadResponseIddefinitely belongs to the correct thread in all cases.
558-559: PassingrerunFromCancelledandpreviousTaskIdto the tracker creation.
This expands the context for re-run tasks. Looks appropriate for re-establishing continuity in the conversation history.
602-602: Cancellation of task delegated toaskingTaskTracker.
This is consistent with the single responsibility principle. Exceptions bubble up as expected, aligning with the agreed-upon error-handling strategy.
610-612: NewgetAskingTaskmethod.
A simple pass-through to the tracker. Error handling at a higher level is consistent with project learnings.
614-615: NewgetAskingTaskByIdmethod.
Mirrors the existing pattern, no issues here. Make sure usage is consistent across code references.
631-631: Using the question as the thread summary.
This is a sensible default, but ensure you revisit if there's a difference between a short summary and a full question in the future.
633-637: StoringaskingTaskIdfromtrackedAskingResulton thread creation.
Good approach for linking the response to the tracker's records. Confirm consumers handle nulltaskIdif the property is undefined.
996-1018: NewgetAskingHistorymethod.
Filters out a canceled response ifexcludeThreadResponseIdis supplied and excludes empty-SQL responses. This is sound for ensuring a consistent conversation flow. Verify that ignoring empty-SQL responses is always desired.
…status change more responsive to client-side
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (1)
433-439: Expand theisResultChangedcheck to handle content changes.Currently, the method only checks status changes. If the result’s data changes while the status remains the same, updates might be missed.
- if (previousResult?.status !== newResult.status) { - return true; - } + if ( + previousResult?.status !== newResult.status || + JSON.stringify(previousResult?.response) !== JSON.stringify(newResult.response) + ) { + return true; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (4)
wren-ui/src/apollo/server/models/adaptor.ts (2) (2)
AskResult(117:131)AskInput(61:66)wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts (1) (1)
IWrenAIAdaptor(46:109)wren-ui/src/apollo/server/repositories/askingTaskRepository.ts (2) (2)
IAskingTaskRepository(23:25)AskingTask(12:21)wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1) (1)
IThreadResponseRepository(54:60)
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts
[error] 144-144: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (3)
140-155: Potential return type mismatch withgetAskingResultFromDB.
getAskingResultFromDBcan returnnull, butgetAskingResultis declared to returnTrackedAskingResult. This is the same discrepancy flagged previously.🧰 Tools
🪛 Biome (1.9.4)
[error] 144-144: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
206-240: Validate concurrency handling during parallel polling.Multiple tasks are processed in parallel with
Promise.allSettled, andrunningJobsis used to guard concurrency. Confirm if external mutation oftrackedTasksor repeated calls to the samequeryIdin quick succession could cause race conditions.Would you like a verification script to grep for concurrent writes or to confirm that Node’s single-threaded event loop is sufficient? Let me know.
411-411: Check for null when assigningtaskId.This line may throw if
this.trackedTasks.get(queryId)isundefined. A null check was previously recommended.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (3)
154-169: 🛠️ Refactor suggestionUpdate
getAskingResultreturn type to match actual behavior.Since this method can return null (when delegating to
getAskingResultFromDB), update the return type to includenullto match actual behavior.- public async getAskingResult(queryId: string): Promise<TrackedAskingResult> { + public async getAskingResult(queryId: string): Promise<TrackedAskingResult | null> {🧰 Tools
🪛 Biome (1.9.4)
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
171-178: 🛠️ Refactor suggestionUpdate
getAskingResultByIdreturn type to match actual behavior.Similar to
getAskingResult, this method can return null but doesn't reflect that in its return type.- public async getAskingResultById(id: number): Promise<TrackedAskingResult> { + public async getAskingResultById(id: number): Promise<TrackedAskingResult | null> {
425-427: 🛠️ Refactor suggestionCheck for null when updating tracked task ID.
Performing
this.trackedTasks.get(queryId).taskId = task.id;can throw an error ifthis.trackedTasks.get(queryId)isundefined. Add a conditional check before assigning the task ID.- this.trackedTasks.get(queryId).taskId = task.id; + const existingTask = this.trackedTasks.get(queryId); + if (existingTask) { + existingTask.taskId = task.id; + }
🧹 Nitpick comments (3)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (3)
20-29: Consider renamingtaskIdtopipelineIdfor consistency with requested changes.The PR objectives mention changing
taskIdtopipeline_id. To ensure consistency with this naming convention, rename the property in theTrackedTaskinterface.interface TrackedTask { queryId: string; - taskId?: number; + pipelineId?: number; lastPolled: number; question?: string; result?: AskResult; isFinalized: boolean; threadResponseId?: number; rerunFromCancelled?: boolean; }
31-35: AligntaskIdinTrackedAskingResultwith the same naming convention change.For consistency with the previous change, also update the
taskIdproperty in theTrackedAskingResulttype.export type TrackedAskingResult = AskResult & { - taskId?: number; + pipelineId?: number; queryId: string; question: string; };
158-165: Use optional chaining for safer property access.Implement optional chaining for
trackedTask.resultto make the code more robust against potentialundefinedvalues.- if (trackedTask && trackedTask.result) { + if (trackedTask?.result) {🧰 Tools
🪛 Biome (1.9.4)
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ui/src/apollo/server/services/askingService.ts(11 hunks)wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
wren-ui/src/apollo/server/services/askingService.ts (2)
Learnt from: wwwy3y3
PR: Canner/WrenAI#1388
File: wren-ui/src/apollo/server/services/askingService.ts:599-604
Timestamp: 2025-03-17T19:26:10.835Z
Learning: Error handling for non-existent asking tasks in WrenAI is handled at the resolver level rather than in the service implementation.
Learnt from: wwwy3y3
PR: Canner/WrenAI#1388
File: wren-ui/src/apollo/server/services/askingService.ts:586-604
Timestamp: 2025-03-17T19:32:09.267Z
Learning: In WrenAI's askingService.ts, errors should be allowed to bubble up to the top-level error handler rather than being caught and logged at the method level.
🧬 Code Definitions (1)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (4)
wren-ui/src/apollo/server/models/adaptor.ts (2) (2)
AskResult(117:131)AskInput(61:66)wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts (1) (1)
IWrenAIAdaptor(46:109)wren-ui/src/apollo/server/repositories/askingTaskRepository.ts (2) (2)
IAskingTaskRepository(23:25)AskingTask(12:21)wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1) (1)
IThreadResponseRepository(54:60)
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (1)
379-403: 🛠️ Refactor suggestionUpdate return type to include null for better type safety.
The method
getAskingResultFromDBreturnsnullwhen no task record is found, but the return type doesn't reflect this possibility. This mismatch could lead to runtime errors when consuming code expects a non-null value.- private async getAskingResultFromDB({ - queryId, - taskId, - }: { - queryId?: string; - taskId?: number; - }): Promise<TrackedAskingResult | null> { + private async getAskingResultFromDB({ + queryId, + taskId, + }: { + queryId?: string; + taskId?: number; + }): Promise<TrackedAskingResult | null> {Likely an incorrect or invalid review comment.
wren-ui/src/apollo/server/services/askingService.ts (6)
567-598: Well-implemented rerun functionality with proper error handling.The new
rerunAskingTaskmethod efficiently reuses the existing task creation logic, properly sets up all required parameters, and includes appropriate error handling when a thread response isn't found.
537-565: Clean integration with the new task tracker.The updated
createAskingTaskimplementation properly delegates to the new askingTaskTracker service and handles the additional parameters for rerunning tasks from cancelled state. The code correctly sets up thread history for follow-up questions.
997-1019: Improved history filtering for rerun scenarios.The enhancement to
getAskingHistoryto exclude specific thread responses (particularly for rerun scenarios) is a valuable improvement that prevents including cancelled responses in the history.
634-649: Thread creation now correctly handles task tracking.The thread creation process has been updated to work with the new task tracking system, properly binding thread responses to asking tasks. This is a clean implementation that maintains the same functionality while transitioning to the new architecture.
690-705: Thread response creation mirrors the thread creation pattern.The
createThreadResponsemethod follows the same pattern ascreateThreadfor binding asking tasks, which maintains consistency in the codebase. This is a good example of the DRY principle.
600-617:Details
✅ Verification successful
Updated task management methods to use askingTaskTracker.
The
cancelAskingTask,getAskingTask, andgetAskingTaskByIdmethods now correctly delegate to the askingTaskTracker service while maintaining telemetry and error handling. This is aligned with the existing pattern of allowing errors to bubble up to the top-level handler rather than catching them at the service level.
🌐 Web query:
What's the best practice for error handling in TypeScript service methods - catching errors at the service level or letting them bubble up?💡 Result:
Error handling in TypeScript service methods requires a strategic balance between localized handling and controlled propagation. Here's a structured approach based on industry practices:
Core Strategy
Handle expected errors at the service level
- Use explicit error types for known failure scenarios
- Return typed results using
Result<T, E>patterns ([3][12])class UserService { async getUser(id: number): Promise<Result<User, DomainError>> { try { return Ok(await db.fetchUser(id)); } catch (e) { if (e instanceof DatabaseError) return Err(new UserNotFound()); return Err(new ServiceUnavailable()); } } }Bubble unexpected errors to boundaries
- Let infrastructure/network errors propagate to centralized handlers ([7][9])
class PaymentGateway { async chargeCard(card: Card) { // No try/catch - let fetch errors bubble const response = await fetch(paymentEndpoint, card); return processResponse(response); } }Best Practice Implementation
Error Boundaries
// Global error handler process.on('uncaughtException', (error) => { logger.critical('Unhandled error', error); metrics.trackFailure(error); }); // API Boundary app.use((err, req, res, next) => { if (err instanceof HttpError) { res.status(err.code).json({ error: err.message }); } else { res.status(500).json({ error: 'System failure' }); } });Critical Components
Aspect Service Level Handling Bubbling Strategy Error Types Domain-specific errors Infrastructure failures Handling Method Explicit conditional checks Top-level boundaries Return Values Typed result objects Unmodified exceptions Logging Contextual logging Generic error capture Key Recommendations
- Adopt error hierarchies using custom
ApplicationErrorclasses ([4][10])- Validate inputs thoroughly before processing ([1][4])
- Implement retry logic for transient errors ([5][7])
- Use type guards for error discrimination
function isDomainError(e: unknown): e is DomainError { return e instanceof DomainError; }The optimal approach combines explicit error handling for business-rule violations with controlled propagation of system-level exceptions. Service methods should manage errors they can meaningfully address, while allowing unexpected failures to bubble to architectural boundaries equipped with logging, monitoring, and standardized responses[5][7][9].
This strategy ensures type safety through the type system (
Resulttypes) while maintaining operational transparency via error boundaries. Developers gain compile-time validation for expected errors, while operations teams get centralized visibility into unanticipated failures.Citations:
- 1: https://www.zipy.ai/blog/typescript-errors
- 2: https://smithy.io/2.0/languages/typescript/ts-ssdk/error-handling.html
- 3: https://blog.logrocket.com/improve-error-handling-typescript-exhaustive-type-checking/
- 4: https://www.dhiwise.com/post/typescript-error-handling-pitfalls-and-how-to-avoid-them
- 5: https://blogs.perficient.com/2024/06/26/understanding-error-handling-in-typescript-strategies-and-best-practices/
- 6: https://dev.to/anttispitkanen/typescript-why-you-shouldnt-throw-errors-to-control-the-program-flow-3g91
- 7: https://clouddevs.com/typescript/error-handling-strategies/
- 8: https://dev.to/mpiorowski/typescript-with-gorust-errors-no-trycatch-heresy-49mf
- 9: https://blog.rampatra.com/how-to-bubble-up-errors-or-exceptions-from-one-method-to-another-in-typescript
- 10: https://engineering.udacity.com/handling-errors-like-a-pro-in-typescript-d7a314ad4991
- 11: https://dev.to/noah-00/no-more-trycatch-a-better-way-to-handle-errors-in-typescript-5hbd
- 12: https://dev.to/noah-00/no-more-trycatch-a-better-way-to-handle-errors-in-typescript-5hbd/comments
- 13: https://www.youtube.com/watch?v=1U44SbOa7rE
Updated Task Management Methods — Implementation Validated
The changes in the methods now appropriately delegate work to the askingTaskTracker service. In particular, the
cancelAskingTaskmethod captures errors only to log telemetry details before rethrowing them, ensuring that unexpected errors bubble up to the global handler. This approach is in line with best practices which recommend handling known errors locally (with associated telemetry/diagnostics) and letting unanticipated exceptions propagate. The retrieval methods similarly delegate directly without interfering with error propagation.No further changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (2)
171-178:⚠️ Potential issueMatch the return type to accommodate a null result.
This method may returnnullif the database record is missing. Modify the return type accordingly:-public async getAskingResultById(id: number): Promise<TrackedAskingResult> { +public async getAskingResultById(id: number): Promise<TrackedAskingResult | null> {
20-41: 🛠️ Refactor suggestionRename “taskId” references to “pipelineId” per PR objectives.
The PR objectives mention changingtaskIdtopipeline_id(or a closely aligned camelCase name). Below is an example diff to renametaskId→pipelineIdandpreviousTaskId→previousPipelineIdfor consistency throughout these type definitions.-interface TrackedTask { - queryId: string; - taskId?: number; - lastPolled: number; - question?: string; - result?: AskResult; - isFinalized: boolean; - threadResponseId?: number; - rerunFromCancelled?: boolean; -} +interface TrackedTask { + queryId: string; + pipelineId?: number; + lastPolled: number; + question?: string; + result?: AskResult; + isFinalized: boolean; + threadResponseId?: number; + rerunFromCancelled?: boolean; +} -export type TrackedAskingResult = AskResult & { - taskId?: number; - queryId: string; - question: string; -}; +export type TrackedAskingResult = AskResult & { + pipelineId?: number; + queryId: string; + question: string; +}; -export type CreateAskingTaskInput = AskInput & { - rerunFromCancelled?: boolean; - previousTaskId?: number; - threadResponseId?: number; -}; +export type CreateAskingTaskInput = AskInput & { + rerunFromCancelled?: boolean; + previousPipelineId?: number; + threadResponseId?: number; +};
🧹 Nitpick comments (4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (4)
17-18: Use a more conservative logging level in production.
While setting this logger to'debug'can be useful during development, it may produce excessive logs in production environments, potentially impacting performance and cluttering log outputs.
73-74: Reduce polling frequency for improved performance.
Polling every second can lead to high overhead in production. Consider increasing the interval or making it environment-configurable, for example:- pollingInterval = 1000, // 1 second + pollingInterval = 5000, // 5 seconds
158-164: Use optional chaining for cleaner null checks.
This prevents repetitive conditions and aligns with modern TypeScript best practices.-if (trackedTask && trackedTask.result) { +if (trackedTask?.result) { return { ...trackedTask.result, queryId, question: trackedTask.question, taskId: trackedTask.taskId, }; }🧰 Tools
🪛 Biome (1.9.4)
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
448-459: Check more than just status for result changes.
Currently,isResultChangedonly comparesstatus. If other fields (e.g., generated SQL) could change, consider a deeper comparison.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (4)
wren-ui/src/apollo/server/models/adaptor.ts (2) (2)
AskResult(117:131)AskInput(61:66)wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts (1) (1)
IWrenAIAdaptor(46:109)wren-ui/src/apollo/server/repositories/askingTaskRepository.ts (2) (2)
IAskingTaskRepository(23:25)AskingTask(12:21)wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1) (1)
IThreadResponseRepository(54:60)
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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/apollo/server/services/askingTaskTracker.ts (1)
43-54: Verify parameter naming alignments inIAskingTaskTracker.
IfidingetAskingResultById(id: number)corresponds to a “pipelineId,” consider renaming it to match. Otherwise, clarify its usage to avoid confusion with newly introduced “pipelineId” naming.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (2)
154-183: Enhanced Retrievability of Task Results
- The distinction between in-memory tasks via
trackedTasksand database fallback throughgetAskingResultFromDBis well-structured.- A minor lint suggestion (from static analysis) indicates that line 160 could use an optional chain (
if (trackedTask?.result)) instead of chained&&. This is stylistic rather than functional.-if (trackedTask && trackedTask.result) { +if (trackedTask?.result) {🧰 Tools
🪛 Biome (1.9.4)
[error] 160-160: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
414-450: Upsert-Like Behavior inupdateTaskInDatabase
- Creating a new record if a task isn’t found is intuitive for your flow.
- Quick in-memory sync ensures subsequent lookups are consistent.
Consider adding some traces or metrics for easier debugging if creation happens unexpectedly, though not strictly necessary.
wren-ui/src/apollo/server/services/askingService.ts (1)
626-627: Comments for Multi-step Thread CreationThe inline comment clarifies the creation flow. Good to see code-based documentation for your multi-step logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ui/src/apollo/server/services/askingService.ts(11 hunks)wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
wren-ui/src/apollo/server/services/askingService.ts (2)
Learnt from: wwwy3y3
PR: Canner/WrenAI#1388
File: wren-ui/src/apollo/server/services/askingService.ts:599-604
Timestamp: 2025-03-17T19:26:10.835Z
Learning: Error handling for non-existent asking tasks in WrenAI is handled at the resolver level rather than in the service implementation.
Learnt from: wwwy3y3
PR: Canner/WrenAI#1388
File: wren-ui/src/apollo/server/services/askingService.ts:586-604
Timestamp: 2025-03-17T19:32:09.267Z
Learning: In WrenAI's askingService.ts, errors should be allowed to bubble up to the top-level error handler rather than being caught and logged at the method level.
🧬 Code Definitions (1)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (4)
wren-ui/src/apollo/server/models/adaptor.ts (2) (2)
AskResult(117:131)AskInput(61:66)wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts (1) (1)
IWrenAIAdaptor(46:109)wren-ui/src/apollo/server/repositories/askingTaskRepository.ts (2) (2)
IAskingTaskRepository(23:25)AskingTask(12:21)wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1) (1)
IThreadResponseRepository(54:60)
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts
[error] 160-160: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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/apollo/server/services/askingTaskTracker.ts (10)
1-19: Imports and Logger Configuration Look GoodAll necessary dependencies are imported, and the logger's debug level is appropriate for monitoring. No issues spotted in these lines.
20-36: Interfaces Provide Clear Structure
TrackedTaskandTrackedAskingResultestablish a helpful contract for in-memory tracking and returning data to clients. Ensuring consistent usage of these types across the codebase will enhance clarity.
37-55: Interface Cohesion
CreateAskingTaskInputandIAskingTaskTrackercomprehensively define the new task creation flow and the supportive methods for retrieving/canceling tasks. The approach aligns well with typical domain-driven design patterns.
56-153: Solid Construction and Task Creation Logic
- The constructor adequately sets default values for
pollingIntervalandmemoryRetentionTime, and it starts polling immediately.- The
createAskingTaskmethod correctly:
- Calls the AI service to create a task.
- Validates parameters for reruns.
- Stores the task in memory for quick polling and updates.
- Error handling is wrapped in a try/catch to avoid leaking partial states.
No immediate critical issues found. The structure is modular and clear.
184-216: Cancellation and Thread Binding
cancelAskingTaskgracefully offloads to thewrenAIAdaptor.bindThreadResponserobustly updates both in-memory and database references. Throwing an error if the task is not found is good practice.All logic is consistent. No immediate problems noted.
218-362: Polling Mechanism is Well-Implemented
- Regular polling with a clear interval ensures timely updates.
- The approach of batching tasks into
jobsand usingPromise.allSettledis effective in a Node.js context.- Cleanup logic to remove stale tasks from memory helps avoid leaks.
All steps appear to handle concurrency in a single-thread environment properly. Great job keeping logic self-contained.
364-386: Thread Response Update on Finalization
- Cleanly extracts the SQL statement from the assigned
viewifviewIdis present.- Graceful fallback to direct SQL from
result.responseif noviewis used.- No evident concurrency or error-handling gaps.
388-412: Database Fallback Retrieval
getAskingResultFromDBproperly returnsnullif no record is found, aligning with your type signature (Promise<TrackedAskingResult | null>).- Logic merges the DB record with partial
detaildata for consistent usage.All good.
452-459: Finalizing Task Status
isTaskFinalizedneatly centralizes the logic to check for finishing states.- Maintains reliability and avoids repeating the same status checks in multiple places.
460-471: Determining Result Changes
isResultChangedfocuses on thestatusdifference, which is a straightforward approach.- For complex result comparisons, consider hashing or a deep compare if needed in the future.
No immediate adjustments needed here.
wren-ui/src/apollo/server/services/askingService.ts (21)
28-28: IntroduceProjectType in Existing ImportBringing in
Projectfromrepositoriesaligns well with references in the service. Nothing problematic here.
38-38: New Types fromaskingTaskTrackerImporting
IAskingTaskTrackerandTrackedAskingResultis consistent with the new architecture. Good job maintaining modular boundaries.
63-63: Track Asking Result Instead ofviewIdReplacing the older approach with
trackedAskingResultfosters a clearer link between the task’s progress and UI or thread updates.
101-115: EnhancedcreateAskingTaskSignatureAdding
rerunFromCancelled,previousTaskId, andthreadResponseIdpaves the way for robust re-run flows. The inline comments clarify usage. Great approach to incorporate backward references while preserving the existing interface shape.
118-119: Return Type Improvement
getAskingTaskandgetAskingTaskByIdnow returnTrackedAskingResult, ensuring consistent typed responses. This is beneficial for front-end consumption.
377-378: New Tracker Property
private askingTaskTracker: IAskingTaskTracker;is introduced and injected via the constructor, improving testability and adherence to the single-responsibility principle.
389-390: Dependency Injection ofaskingTaskTrackerEnsuring that the tracker is provided at instantiation aligns with the overall architecture. No issues noted.
540-542: Additional Parameters for Re-runIntroducing
rerunFromCancelled,previousTaskId, andthreadResponseIdensures fine-grained control for canceled tasks. This design feels intuitive.
550-552: Fetching History with Exclusions
getAskingHistoryis being utilized to exclude canceled thread responses if needed. Handling previous queries for re-runs is well-executed.
600-600: Cancellation DelegationThe
cancelAskingTaskwraps and delegates toaskingTaskTracker. Clear layering. Telemetry logging is consistent with existing patterns.
611-613: Nullable Return forgetAskingTaskExplicitly returning
Promise<TrackedAskingResult | null>helps mitigate runtime exceptions if the record is absent. This also matches prior design patterns of letting the resolver handle the missing-task scenario.
617-619: NewgetAskingTaskByIdThis similarly returns
nullif the record cannot be found. Consistency withgetAskingTaskis appreciated.
642-643: Linking ThreadResponse and TaskThe property
askingTaskIdis assigned fromtrackedAskingResult?.taskId, ensuring proper mapping. The logic is straightforward.
646-652: Binding the Task after CreationBy calling
askingTaskTracker.bindThreadResponseiftrackedAskingResultexists, you keep the data sources in sync. This ensures immediate reference consistency.
683-684: AssigningaskingTaskIdDuring Response CreationSimilar logic to the thread creation step. The code remains consistent. Good approach to unify how tasks link to responses.
698-699: Tracking Possibly Non-Null Task IDAdhering to
trackedAskingResult?.taskIdensures the field remains optional. No problems spotted.
701-708: Binding Task increateThreadResponseRepeats the pattern from thread creation. This uniform approach fosters maintainability.
1001-1004: EnhancedgetAskingHistoryIncluding an optional
excludeThreadResponseIdparameter eliminates unwanted queries and duplicates for re-runs. Logical and well-structured retrieval.
1008-1011: Limiting History to 10 ResponsesA practical approach to reduce overhead. It helps avoid flooding the system with large histories.
1013-1018: Filtering Out Canceled or Duplicate ResponsesDynamically excluding a canceled thread response keeps the context relevant for re-runs. Proper design choice.
1021-1022: SQL Filter for Non-empty ExpressivenessFinal filtering ensures the AI sees only valid SQL. This helps mitigate confusion from incomplete or empty statements.
2 participants
Description
taskId, then asking tasks will be tracked at background.askingTaskquery or from the fieldaskingTaskof thread/thread responseHow to use new API
Create thread with taskId
CreateThreadResponsemutation has the sametaskIdparameter.Get asking task from thread response
Use recommended question
Just provide
questionandsql.Rerun asking task
Summary by CodeRabbit
New Features
Refactor
Chores