feat(wren-ui): Fix SQL flow in reasoning#1451
Conversation
WalkthroughThis PR introduces a new GraphQL mutation, Changes
Sequence Diagram(s)sequenceDiagram
participant UI as HomeThread UI
participant Mutation as useUpdateThreadResponseMutation
participant Resolver as updateThreadResponse Resolver
participant Service as AskingService
UI->>Mutation: onFixSQLStatement(responseId, sql)
Mutation->>Resolver: Execute updateThreadResponse mutation
Resolver->>Service: Call updateThreadResponse(responseId, {sql})
Service-->>Resolver: Return updated ThreadResponse
Resolver-->>Mutation: Return response data
Mutation-->>UI: Update UI with new response
sequenceDiagram
participant EB as ErrorBoundary
participant Modal as FixSQLModal
participant UI as User
EB->>UI: Display error with "Fix it" option
UI->>EB: Click "Fix it" button
EB->>Modal: Open FixSQLModal for SQL fix
UI->>Modal: Input and submit corrected SQL
Modal->>EB: Invoke fixStatement(sql)
EB->>Mutation: Execute updateThreadResponse mutation
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
wren-ui/src/apollo/server/services/askingService.ts (1)
718-725: Consider adding telemetry tracking for SQL updates.The implementation is clean and straightforward, but unlike many other methods in this service that include telemetry tracking, this method doesn't. Consider adding telemetry to maintain consistency with the pattern used elsewhere in the codebase.
public async updateThreadResponse( responseId: number, data: { sql: string }, ): Promise<ThreadResponse> { + const eventName = TelemetryEvent.HOME_UPDATE_THREAD_RESPONSE; + try { const response = this.threadResponseRepository.updateOne(responseId, { sql: data.sql, }); + this.telemetry.sendEvent(eventName, { responseId }); return response; + } catch (err: any) { + this.telemetry.sendEvent( + eventName, + { responseId, error: err.message }, + err.extensions?.service, + false + ); + throw err; + } }wren-ui/src/apollo/server/resolvers/askingResolver.ts (1)
422-431: Add error handling and telemetry tracking for consistency.While the implementation is correct, it lacks error handling and telemetry tracking that is present in similar methods like
createThreadResponse. Consider adding these for consistency and easier debugging.public async updateThreadResponse( _root: any, args: { where: { id: number }; data: { sql: string } }, ctx: IContext, ): Promise<ThreadResponse> { const { where, data } = args; const askingService = ctx.askingService; + const eventName = TelemetryEvent.HOME_UPDATE_THREAD_RESPONSE; + try { const response = await askingService.updateThreadResponse(where.id, data); + ctx.telemetry.sendEvent(eventName, { responseId: where.id }); return response; + } catch (err: any) { + ctx.telemetry.sendEvent( + eventName, + { responseId: where.id, error: err.message }, + err.extensions?.service, + false + ); + throw err; + } }wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (1)
6-6: Potential naming conflict with global Error object.The static analysis tool has flagged that importing the
Errortype shadows the globalErrorproperty which could lead to confusion.Consider renaming the imported type to avoid shadowing:
-import { Error } from '@/apollo/client/graphql/__types__'; +import { Error as GraphQLError } from '@/apollo/client/graphql/__types__';And update the usage throughout the file:
-error?: Error & { +error?: GraphQLError & {🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
wren-ui/src/components/modals/FixSQLModal.tsx (2)
31-36: Consider adding more robust SQL validation.The
validateSqlfunction performs basic validation but doesn't handle edge cases beyond what the GraphQL mutation returns.You could enhance this by adding client-side validation patterns for common SQL errors before sending the request to the server:
const validateSql = async () => { const sql = form.getFieldValue('sql'); + // Check for basic SQL syntax patterns + const basicValidation = /SELECT|INSERT|UPDATE|DELETE|CREATE|DROP/i.test(sql); + if (!basicValidation) { + return Promise.reject(new Error('SQL must contain a valid statement type')); + } await previewSqlMutation({ variables: { data: { sql, limit: 1, dryRun: true } }, }); };
103-109: Add confirmation before submitting with preview errors.The preview button is currently disabled while loading, but you could enhance UX by adding a warning if the user tries to submit after a preview error.
<Button onClick={previewData} loading={previewSqlResult.loading} disabled={previewSqlResult.loading} + danger={!!previewSqlResult.error} > - Preview data + {previewSqlResult.error ? 'Retry Preview' : 'Preview data'} </Button>wren-ui/src/apollo/client/graphql/__types__.ts (1)
1295-1297: Consider adding more fields to UpdateThreadResponseInput.Currently, the input only allows updating the SQL field, which is appropriate for the current feature but might need expansion in the future.
export type UpdateThreadResponseInput = { sql?: InputMaybe<Scalars['String']>; + // Consider adding other fields that might need updating in the future + // question?: InputMaybe<Scalars['String']>; + // Other fields as needed... };wren-ui/src/components/pages/home/preparation/PreparationSteps.tsx (4)
17-28: Consider moving styled components to a separate file.As the codebase grows, keeping styled components in separate files can improve maintainability.
Consider moving styled components to a separate file like
styled.tsfor better organization:// styled.ts import styled from 'styled-components'; import { Badge } from 'antd'; export const StyledBadge = styled(Badge)` position: absolute; top: -5px; left: -3px; .ant-badge-status-dot { width: 7px; height: 7px; } .ant-badge-status-text { display: none; } `;
49-81: Consider splitting complex logic into smaller functions.The component has several computed variables and conditions. Breaking these into smaller functions would improve readability.
export default function PreparationSteps(props: Props) { const { className, data, askingStreamTask, isAnswerFinished } = props; const { askingTask, view, sql } = data; const processState = useMemo( () => convertAskingTaskToProcessState(askingTask), [askingTask], ); const isFixedSQL = useMemo(() => { return sql && askingTask?.invalidSql; }, [sql, askingTask?.invalidSql]); + // Extract display logic to separate functions + const getDisplayStates = () => { + const showView = !!view; + const showRetrieving = + retrievingNextStates.includes(processState) && !showView; + const showOrganizing = + organizingNextStates.includes(processState) && !showView; + const showGenerating = + generatingNextStates.includes(processState) && !showView; + + return { showView, showRetrieving, showOrganizing, showGenerating }; + }; + + const getLoadingStates = () => { + const retrieving = processState === PROCESS_STATE.SEARCHING; + const organizing = processState === PROCESS_STATE.PLANNING; + const generating = processState === PROCESS_STATE.GENERATING; + const correcting = processState === PROCESS_STATE.CORRECTING; + const wrapping = !isAnswerFinished; + + return { retrieving, organizing, generating, correcting, wrapping }; + }; - // displays - const showView = !!view; - const showRetrieving = - retrievingNextStates.includes(processState) && !showView; - const showOrganizing = - organizingNextStates.includes(processState) && !showView; - const showGenerating = - generatingNextStates.includes(processState) && !showView; - - // data + // Extract data from props const retrievedTables = askingTask.retrievedTables || []; const sqlGenerationReasoning = askingTask.sqlGenerationReasoning || askingStreamTask || ''; - // loadings - const retrieving = processState === PROCESS_STATE.SEARCHING; - const organizing = processState === PROCESS_STATE.PLANNING; - const generating = processState === PROCESS_STATE.GENERATING; - const correcting = processState === PROCESS_STATE.CORRECTING; - const wrapping = !isAnswerFinished; + const { showView, showRetrieving, showOrganizing, showGenerating } = getDisplayStates(); + const { retrieving, organizing, generating, correcting, wrapping } = getLoadingStates();
112-117: Consider using a constant for the icon styling.The inline styling could be moved to a styled component or constant.
-const fileDone = ( - <FileDoneOutlined - className="gray-6" - style={{ position: 'relative', top: -2, left: 2 }} - /> -); +const StyledFileDoneIcon = styled(FileDoneOutlined)` + position: relative; + top: -2px; + left: 2px; +`; + +const fileDone = <StyledFileDoneIcon className="gray-6" />;
119-129: Duplicate Timeline structure in specialized components.
FixedSQLTimelineStepsandViewTimelineStepshave identical structure with different content components. Consider refactoring to reduce duplication.-function FixedSQLTimelineSteps(props: Props) { - const { className } = props; - - return ( - <Timeline className={className}> - <Timeline.Item dot={fileDone}> - <FixedSQLFinished /> - </Timeline.Item> - </Timeline> - ); -} - -function ViewTimelineSteps(props: Props) { - const { className } = props; - - return ( - <Timeline className={className}> - <Timeline.Item dot={fileDone}> - <ViewFinished /> - </Timeline.Item> - </Timeline> - ); -} +function SingleStepTimeline(props: { className?: string; children: React.ReactNode }) { + const { className, children } = props; + + return ( + <Timeline className={className}> + <Timeline.Item dot={fileDone}> + {children} + </Timeline.Item> + </Timeline> + ); +} + +function FixedSQLTimelineSteps(props: Props) { + return ( + <SingleStepTimeline className={props.className}> + <FixedSQLFinished /> + </SingleStepTimeline> + ); +} + +function ViewTimelineSteps(props: Props) { + return ( + <SingleStepTimeline className={props.className}> + <ViewFinished /> + </SingleStepTimeline> + ); +}Also applies to: 131-141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
wren-ui/package.json(1 hunks)wren-ui/src/apollo/client/graphql/__types__.ts(4 hunks)wren-ui/src/apollo/client/graphql/home.generated.ts(2 hunks)wren-ui/src/apollo/client/graphql/home.ts(1 hunks)wren-ui/src/apollo/server/resolvers.ts(1 hunks)wren-ui/src/apollo/server/resolvers/askingResolver.ts(2 hunks)wren-ui/src/apollo/server/schema.ts(2 hunks)wren-ui/src/apollo/server/services/askingService.ts(2 hunks)wren-ui/src/components/modals/FixSQLModal.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/PreparationStatus.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/PreparationSteps.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/index.tsx(2 hunks)wren-ui/src/components/pages/home/preparation/step/FixedSQLFinished.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Organizing.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/ViewFinished.tsx(1 hunks)wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx(3 hunks)wren-ui/src/components/pages/home/promptThread/store.tsx(1 hunks)wren-ui/src/pages/home/[id].tsx(4 hunks)wren-ui/src/styles/components/button.less(1 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
wren-ui/src/apollo/server/services/askingService.ts (2)
wren-ui/src/apollo/client/graphql/__types__.ts (1)
ThreadResponse(1158-1169)wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1)
ThreadResponse(42-52)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (2)
wren-ui/src/apollo/client/graphql/__types__.ts (1)
Error(427-433)wren-ui/src/components/modals/FixSQLModal.tsx (1)
FixSQLModal(15-130)
wren-ui/src/apollo/server/resolvers/askingResolver.ts (2)
wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1)
ThreadResponse(42-52)wren-ui/src/apollo/client/graphql/__types__.ts (1)
ThreadResponse(1158-1169)
wren-ui/src/apollo/client/graphql/__types__.ts (1)
wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1)
ThreadResponse(42-52)
wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx (2)
wren-ui/src/components/pages/home/promptThread/TextBasedAnswer.tsx (1)
getAnswerIsFinished(25-30)wren-ui/src/components/pages/home/preparation/index.tsx (1)
Preparation(21-87)
wren-ui/src/apollo/client/graphql/home.generated.ts (1)
wren-ui/src/apollo/client/graphql/__types__.ts (3)
Exact(4-4)ThreadResponseUniqueWhereInput(1210-1212)UpdateThreadResponseInput(1295-1297)
wren-ui/src/components/pages/home/preparation/PreparationSteps.tsx (3)
wren-ui/src/components/pages/home/preparation/index.tsx (1)
Props(15-19)wren-ui/src/components/pages/home/preparation/step/FixedSQLFinished.tsx (1)
FixedSQLFinished(3-15)wren-ui/src/components/pages/home/preparation/step/ViewFinished.tsx (1)
ViewFinished(3-12)
wren-ui/src/pages/home/[id].tsx (3)
wren-ui/src/apollo/server/resolvers/askingResolver.ts (1)
updateThreadResponse(422-431)wren-ui/src/apollo/server/services/askingService.ts (1)
updateThreadResponse(718-725)wren-ui/src/apollo/client/graphql/home.generated.ts (1)
useUpdateThreadResponseMutation(719-722)
🪛 Biome (1.9.4)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx
[error] 6-6: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (48)
wren-ui/src/components/pages/home/preparation/step/Organizing.tsx (1)
41-41: Improved loading spinner behavior.This conditional change ensures that the spinner is only displayed when loading is in progress and no stream content is available. When stream data is present, it will be displayed immediately instead of being hidden behind a loading spinner, enhancing the user experience.
wren-ui/package.json (1)
105-106: LGTM: Added newline at end of file.This follows standard best practices for text files in many development environments.
wren-ui/src/utils/error/dictionary.ts (1)
111-115: LGTM: Added new error message for SQL fixes.The new error message structure for the "Fix SQL" feature follows the same pattern as existing error messages, maintaining consistency throughout the codebase.
wren-ui/src/styles/components/button.less (1)
17-26: LGTM: Added styled button for the Fix SQL feature.The new
.adm-fix-it-btnclass provides distinct styling for the SQL fix functionality, with appropriate hover and focus states for user interaction feedback.wren-ui/src/components/pages/home/promptThread/store.tsx (1)
19-19: Interface extension provides SQL fix capabilityThe addition of
onFixSQLStatementto theIPromptThreadStoreinterface properly extends the preparation functionality to support manual SQL fixes. This aligns with the PR's objective of introducing a "Fix SQL" flow for when AI-generated SQL fails.wren-ui/src/components/pages/home/preparation/step/ViewFinished.tsx (1)
1-12: Clean implementation of pre-saved view feedback componentThis new component clearly communicates to users when a pre-saved view is being used, which enhances the overall user experience by providing immediate feedback about query execution status.
wren-ui/src/apollo/server/resolvers.ts (1)
101-101: Added mutation resolver for thread response updatesThe new
updateThreadResponseresolver is appropriately positioned in the Mutation section following the existingcreateThreadResponse. This addition is essential for implementing the "Fix SQL" flow by enabling SQL updates in thread responses.wren-ui/src/components/pages/home/preparation/step/FixedSQLFinished.tsx (1)
1-15: Clear user feedback for manually fixed SQLThis component effectively communicates that user-provided SQL is being used, explaining the reason (system encountered an issue) and current status (query is being processed). The messaging is clear and informative.
wren-ui/src/apollo/client/graphql/home.ts (1)
238-248: LGTM! The newUPDATE_THREAD_RESPONSEGraphQL mutation is well-structured.The mutation is properly formatted with appropriate arguments and correctly reuses the
COMMON_RESPONSEfragment for a consistent response structure.wren-ui/src/apollo/server/services/askingService.ts (1)
135-139: Interface addition looks good.The new method signature is appropriately defined in the interface with clear parameter types.
wren-ui/src/apollo/server/schema.ts (2)
640-646: Schema input types are well-defined.The new input types
ThreadResponseUniqueWhereInputandUpdateThreadResponseInputare properly structured with appropriate field types.
1017-1020: GraphQL mutation signature looks good.The
updateThreadResponsemutation is properly defined with the appropriate parameters and return type, consistent with other mutations in the schema.wren-ui/src/apollo/server/resolvers/askingResolver.ts (1)
87-87: Method binding in constructor is properly placed.The binding of the new
updateThreadResponsemethod is correctly added in the constructor alongside other method bindings.wren-ui/src/pages/home/[id].tsx (4)
33-33: Import added for SQL updating functionality.The import of
useUpdateThreadResponseMutationis necessary to support the new SQL correction feature.
116-122: Well-implemented mutation with appropriate success handling.The mutation implementation includes a success message and correctly triggers the generation of a new answer after SQL update. This ensures that users get immediate feedback and updated results after fixing SQL.
181-185: Clean implementation of the SQL fix function.The
onFixSQLStatementfunction is concise and follows the established patterns for mutations in the codebase. It correctly passes the response ID and updated SQL to the mutation.
320-320: Proper exposure of the SQL fix function to child components.Adding
onFixSQLStatementto the provider value makes it accessible to all child components via the context, which is necessary for this feature to work throughout the component tree.wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx (4)
189-189: Simplified logic for determining if an answer is prepared.The condition has been simplified to check if either
queryIdorstatusexists, which is cleaner and more maintainable.
234-235: Added clearer distinction between prepared and finished states.The new
isAnswerFinishedvariable provides a more accurate representation of completion state by combining the preparation status with the finished status check.
244-244: Updated prop to use the more accurate finished state.Changing from
isAnswerPreparedtoisAnswerFinishedensures the Preparation component can correctly determine when to stop displaying or collapse itself.
248-257: Improved JSX readability by removing unnecessary fragments.The removal of fragment wrappers around tab content improves code readability while maintaining the same functionality.
Also applies to: 265-269, 276-281
wren-ui/src/components/pages/home/preparation/PreparationStatus.tsx (1)
1-63: Well-structured component for managing preparation status.This new component effectively manages the UI states for task preparation with:
- Clean loading state management
- Proper event handling for cancel and re-run
- Clear conditional rendering based on task status
The component follows React best practices and integrates well with the existing architecture.
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (4)
10-13: Extended error interface to support SQL fixing functionality.The error prop type has been enhanced to include
invalidSqlandfixStatementproperties, which are necessary for the SQL correction feature.
17-19: Added modal management for SQL fixing.The modal action hook and invalid SQL detection are cleanly implemented, supporting the new SQL correction UI flow.
24-33: Improved error messaging with SQL-specific context.The conditional error message provides more specific information when there's an SQL issue, which improves the user experience by setting the right expectations.
34-52: Well-implemented SQL fix UI integration.The implementation:
- Shows a "Fix it" button only when there's invalid SQL
- Integrates with the FixSQLModal component
- Properly passes the SQL correction through the fixStatement callback
This provides users with a seamless way to correct SQL errors when they occur.
wren-ui/src/components/modals/FixSQLModal.tsx (5)
1-14: Well-structured component with clear props definition.The component imports and props definition are clean and clearly communicate the component's purpose. The
ModalActiontype is reused effectively for the modal's behavior pattern.
19-24: Effective error handling with memoization.Good use of
useMemoto prevent unnecessary error object recreation. The error parsing and formatting with a standardized short message improves the user experience.
67-80: Modal configuration is appropriate.The modal configuration is well-designed with appropriate callbacks for closing, submitting, and resetting state when closed.
84-97: Form implementation follows best practices.The form setup with validation rules and proper field configuration ensures data integrity. Using
preserve={false}withresetFieldsis a good practice.
120-127: Error display is clear and user-friendly.The error alert with expandable details via ErrorCollapse provides a good balance of showing important information without overwhelming the user.
wren-ui/src/apollo/client/graphql/__types__.ts (3)
579-579: Good addition of updateThreadResponse mutation.The new mutation is properly added to the Mutation type, maintaining consistency with the existing schema structure.
829-832: MutationUpdateThreadResponseArgs follows schema pattern.The arguments for the mutation follow the established pattern of having
dataandwhereparameters.
1210-1212: ThreadResponseUniqueWhereInput is appropriately defined.The where input type correctly uses the ID as a unique identifier for thread responses.
wren-ui/src/components/pages/home/preparation/index.tsx (6)
4-8: Good component modularity with new imports.The refactoring to use
PreparationStepsas a separate component improves code organization and separation of concerns.
15-19: Well-structured props with updated property name.Changing from
isAnswerPreparedtoisAnswerFinishedmakes the intent clearer.
22-23: Destructuring props improves readability.Proper destructuring of props makes the code more maintainable and easier to understand.
25-30: Simplified state management.The state management is more straightforward now, focusing only on whether the answer is finished.
31-39: Enhanced error handling with SQL fixing capability.The error object now includes a
fixStatementfunction that enables users to correct SQL statements, which is a significant UX improvement.
80-82: Clean integration with PreparationSteps component.The component effectively passes necessary props to the new
PreparationStepscomponent.wren-ui/src/components/pages/home/preparation/PreparationSteps.tsx (4)
1-16: Well-structured imports and component organization.The imports are organized logically, and the component leverages existing step components.
30-47: Well-defined state machine transitions.Using the state machine for process state transitions is a good practice for maintaining clear state logic.
82-85: Early returns for different scenarios improve readability.Using early returns for view and fixed SQL cases makes the code more understandable.
86-109: Timeline component is well-structured.The Timeline component effectively uses conditional rendering to show only relevant steps based on the current state.
wren-ui/src/apollo/client/graphql/home.generated.ts (4)
95-98: Well-defined mutation variables typeThe
UpdateThreadResponseMutationVariablestype properly defines the required variables for the mutation, using the existing type system withThreadResponseUniqueWhereInputto identify the response andUpdateThreadResponseInputto specify the updated data.
101-101: Comprehensive mutation response typeThe
UpdateThreadResponseMutationtype correctly defines the structure of the mutation's response, leveraging the existingCommonResponsefragment pattern to maintain consistency with other mutations in the system.
692-698: Properly structured GraphQL mutation documentThe GraphQL mutation is well-defined and follows the project's established patterns, including the required parameters (
whereanddata) and returning theCommonResponsefragment to ensure a complete response structure.
699-725: Complete Apollo mutation hook implementationThe mutation hook and related types follow Apollo Client's best practices, providing a type-safe way for React components to execute the update thread response mutation. This will enable the UI to properly update SQL queries when needed.
These changes align well with the PR objective to "introduce a Fix SQL flow that enables users to manually input a corrected SQL query when the AI-generated SQL fails or encounters an error."
|
@andreashimin Thanks for your hard work! I am wondering whether ui for sql syntax fixing will include "sql syntax error message"? |
This PR currently only displayed generic message to users, not sure AI service will provide sql syntax error message? If any feature missing, pls correct me |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
wren-ui/src/utils/helper.ts (1)
23-32:⚠️ Potential issueUpdate the implementation to match the new type signature
The type signature for
asyncRequesthas been updated to returnPromise<any>instead ofPromise<void>, which allows this utility to work with functions that return values. However, the current implementation ofattachLoadingdoesn't actually return the result of theasyncRequestcall, making the type change ineffective.To ensure the implementation matches the new type signature, modify the function to return the result:
export const attachLoading = ( asyncRequest: (...args: any[]) => Promise<any>, setLoading: React.Dispatch<React.SetStateAction<boolean>>, ) => { return async (...args) => { setLoading(true); - await asyncRequest(...args); + const result = await asyncRequest(...args); setLoading(false); + return result; }; };This change ensures that any return value from the wrapped async function is properly passed through to the caller, which is likely needed for the new SQL flow functionality.
🧹 Nitpick comments (1)
wren-ui/src/apollo/server/services/askingService.ts (1)
718-732: Implementation looks good, consider adding logging.The method correctly:
- Finds the thread response using the repository
- Validates existence before proceeding
- Updates the SQL field with the user-provided value
For consistency with other methods in this service, consider adding debug logging when updating the thread response.
public async updateThreadResponse( responseId: number, data: { sql: string }, ): Promise<ThreadResponse> { const threadResponse = await this.threadResponseRepository.findOneBy({ id: responseId, }); if (!threadResponse) { throw new Error(`Thread response ${responseId} not found`); } + logger.debug(`Updating SQL for thread response ${responseId}`); return await this.threadResponseRepository.updateOne(responseId, { sql: data.sql, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-ui/src/apollo/server/services/askingService.ts(2 hunks)wren-ui/src/components/modals/FixSQLModal.tsx(1 hunks)wren-ui/src/utils/helper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-ui/src/components/modals/FixSQLModal.tsx
🧰 Additional context used
🧬 Code Definitions (1)
wren-ui/src/apollo/server/services/askingService.ts (2)
wren-ui/src/apollo/client/graphql/__types__.ts (2)
ThreadResponse(1158-1169)Error(427-433)wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1)
ThreadResponse(42-52)
🔇 Additional comments (1)
wren-ui/src/apollo/server/services/askingService.ts (1)
135-138: Well-defined interface method for thread response SQL updates.The addition of the
updateThreadResponsemethod to theIAskingServiceinterface correctly defines the necessary parameters and return type for updating SQL in a thread response.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (2)
7-7: Consider renaming the importedErrortype to avoid shadowing the global Error.Importing
Errorfrom GraphQL types shadows JavaScript's globalErrorobject, which could lead to confusion.-import { Error } from '@/apollo/client/graphql/__types__'; +import { Error as GraphQLError } from '@/apollo/client/graphql/__types__';🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
51-53: Consider adding error handling for the fixStatement function.The current implementation doesn't handle potential failures when calling
error.fixStatement(). Consider adding error handling to improve user experience when SQL updates fail.onSubmit={async (sql: string) => { - await error.fixStatement(sql); + try { + await error.fixStatement(sql); + // Optional: Add success notification + } catch (err) { + // Handle error, perhaps show notification or message + console.error('Failed to update SQL:', err); + } }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-ui/package.json(1 hunks)wren-ui/src/components/ErrorCollapse.tsx(2 hunks)wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wren-ui/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx
[error] 7-7: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
wren-ui/src/components/ErrorCollapse.tsx (3)
30-30: Well-designed property additionThe new optional
defaultActiveproperty enhances the component's flexibility by allowing consumers to specify whether error messages should be initially expanded.
34-37: Proper state management implementationGood implementation of the component's initial state based on the
defaultActiveprop. Using the useState hook with conditional initialization is the right approach here.
43-44: Controlled component pattern correctly implementedThe component now properly implements the controlled component pattern by passing the
activeKeystate to the Collapse component and handling state changes through theonChangehandler.wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (7)
1-7: Good addition of necessary imports for the new SQL fix feature.The imports appropriately include the required components and icons for implementing the SQL fixing functionality. The
FixSQLModalanduseModalActionimports are particularly important for the new interactive feature.🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
9-15: LGTM: Props interface has been properly extended to support SQL fixing.The
fixStatementfunction added to the Props interface is well-typed and aligns with the PR objective of enabling users to manually fix SQL queries.
17-20: Good use of the modal hook for managing the SQL fix modal state.The
useModalActionhook provides a clean way to handle the modal's open/close state and data passing, making the component's logic easier to follow.
25-28: Improved error message display logic based on error type.The conditional rendering provides better user context by displaying a specific message for SQL generation failures instead of using the generic error message.
30-34: Good user guidance for SQL errors with helpful messaging.The error message is now more helpful and actionable for users, clearly explaining the issue and inviting them to help fix the SQL problem.
35-56: Excellent implementation of the SQL fixing UI with clear user guidance.The implementation effectively:
- Shows the error details in a collapsible component
- Provides a "Fix it" button with appropriate icon
- Integrates the SQL fixing modal with proper data passing
- Handles the SQL update via the provided callback
This fulfills the PR objective of allowing users to correct SQL when the AI-generated SQL fails.
38-39:Details
✅ Verification successful
Verify that ErrorCollapse correctly displays SQL error details.
Ensure the
ErrorCollapsecomponent properly formats and displays SQL error messages for better user understanding.
🏁 Script executed:
#!/bin/bash # Check the implementation of ErrorCollapse component cat $(fd --type file --hidden --exec grep -l "ErrorCollapse" {} \;)Length of output: 77190
Verified SQL Error Display in ErrorCollapse Component
The ErrorCollapse implementation uses a preformatted element with “pre-wrap” styling, ensuring that multi-line and detailed SQL error messages are rendered clearly. Its behavior—toggling the panel based on the defaultActive prop—is consistent across its usages in ErrorBoundary and other modals. No modifications are necessary.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
wren-ui/src/components/modals/FixSQLModal.tsx (3)
18-21: Consider addressing UI loading state confusion.As mentioned in previous feedback, there might be confusion with loading states between validation, preview, and submission. Consider managing these states more distinctly to improve user experience.
You could add a separate loading state for validation to differentiate it from preview loading:
const [previewLoading, setPreviewLoading] = useState(false); + const [validateLoading, setValidateLoading] = useState(false);Then use it in the validateSql function:
const validateSql = async () => { const sql = form.getFieldValue('sql'); + setValidateLoading(true); await previewSqlMutation({ variables: { data: { sql, limit: 1, dryRun: true } }, }); + setValidateLoading(false); };And update the showPreview logic to avoid showing preview during validation:
- const showPreview = previewSqlResult.data || previewSqlResult.loading; + const showPreview = previewSqlResult.data || (previewSqlResult.loading && !validateLoading);Also applies to: 70-70
12-14: Improve type definition with documentation.The Props type could benefit from JSDoc comments to explain each property, especially for other developers who might use this component.
-type Props = ModalAction<{ sql: string; responseId: number }> & { - loading?: boolean; -}; +/** + * Props for the FixSQLModal component + */ +type Props = ModalAction<{ + /** The SQL statement to be fixed */ + sql: string; + /** The ID of the response being updated */ + responseId: number +}> & { + /** Whether the submit button is in loading state */ + loading?: boolean; +};
1-11: Consider grouping related imports.The imports could be organized better by grouping them into categories like React, external libraries, components, and utilities.
-import { useEffect, useMemo, useState } from 'react'; -import { Button, Form, Modal, Typography, Alert } from 'antd'; -import { ERROR_TEXTS } from '@/utils/error'; -import { ModalAction } from '@/hooks/useModalAction'; -import { attachLoading } from '@/utils/helper'; -import { parseGraphQLError } from '@/utils/errorHandler'; -import SQLEditor from '@/components/editor/SQLEditor'; -import ErrorCollapse from '@/components/ErrorCollapse'; -import PreviewData from '@/components/dataPreview/PreviewData'; -import { usePreviewSqlMutation } from '@/apollo/client/graphql/sql.generated'; +// React imports +import { useEffect, useMemo, useState } from 'react'; + +// External libraries +import { Button, Form, Modal, Typography, Alert } from 'antd'; + +// Hooks and API +import { ModalAction } from '@/hooks/useModalAction'; +import { usePreviewSqlMutation } from '@/apollo/client/graphql/sql.generated'; + +// Components +import SQLEditor from '@/components/editor/SQLEditor'; +import ErrorCollapse from '@/components/ErrorCollapse'; +import PreviewData from '@/components/dataPreview/PreviewData'; + +// Utilities +import { ERROR_TEXTS } from '@/utils/error'; +import { attachLoading } from '@/utils/helper'; +import { parseGraphQLError } from '@/utils/errorHandler';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-ui/src/components/modals/FixSQLModal.tsx(1 hunks)wren-ui/src/components/modals/QuestionSQLPairModal.tsx(3 hunks)wren-ui/src/utils/helper.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wren-ui/src/components/modals/QuestionSQLPairModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-ui/src/utils/helper.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
wren-ui/src/components/modals/FixSQLModal.tsx (4)
22-26: Good use of useMemo for error parsing.The error parsing logic is efficiently implemented using useMemo to prevent unnecessary recalculations. This is a good practice for performance optimization.
40-52: Improve error handling in previewData function.While you're catching errors with
console.error, it would be better to handle them properly and show feedback to the user.const previewData = async () => { form .validateFields() .then(async (values) => { await attachLoading( previewSqlMutation, setPreviewLoading, )({ variables: { data: { sql: values.sql, limit: 50 } }, }); }) - .catch(console.error); + .catch((error) => { + // Parse the error message + const graphQLError = parseGraphQLError(error); + // Update UI state to show the error + console.error('Preview data error:', graphQLError); + }); };
59-68: Improve error handling in submit function.Similar to the
previewDatafunction, thesubmitfunction should better handle errors.const submit = async () => { form .validateFields() .then(async (values) => { await validateSql(); await onSubmit(values.sql); onClose(); }) - .catch(console.error); + .catch((error) => { + // Parse the error and handle it appropriately + console.error('Submit error:', error); + // No need to close the modal on error + }); };
125-132: Good error display with collapsible details.The use of Alert with ErrorCollapse provides a good user experience for displaying errors. It shows a concise error message with the ability to expand for more details.
andreashimin
force-pushed
the
feat/reasoning-fix-sql
branch
from
dce62a8 to
ad5f5e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
wren-ui/src/components/pages/home/preparation/PreparationStatus.tsx (1)
29-60: Conditional UI returns appear correct, but consider clarifying the steps text.The tiered checks for
isProcessing,STOPPED, andFINISHEDare clear, and the respective UI elements are well-defined. However, returning'1 step'vs.'3 steps'(line 59) may cause confusion if these are placeholders or purely indicative. Clarify or make them dynamic if they represent actual step counts.wren-ui/src/components/pages/home/preparation/PreparationSteps.tsx (1)
49-109: Comprehensive and clear timeline rendering logic.The approach of branching for
showView,isFixedSQL, and default steps is intuitive. Each staging function (Retrieving,Organizing,Generating) receives the correct loading states. Minor note: thewrappingvariable name at line 80 might be more descriptive asisWrappingor similar, but it does not impact functionality.wren-ui/src/apollo/server/services/askingService.ts (1)
718-732: Consider adding await for improved readabilityThe implementation is solid with proper error handling when the thread response is not found. While functionally correct without the
awaitkeyword since the function is already marked asasync, adding it would improve readability by making the asynchronous nature more explicit.- return this.threadResponseRepository.updateOne(responseId, { + return await this.threadResponseRepository.updateOne(responseId, {wren-ui/src/pages/home/[id].tsx (1)
116-122: Consider adding error handling to the mutationThe mutation hook includes good success handling with a user-friendly message and follow-up action. However, it lacks explicit error handling which could improve the user experience when errors occur.
const [updateThreadResponse] = useUpdateThreadResponseMutation({ onCompleted: (data) => { message.success('Successfully updated the SQL statement'); // trigger generate answer after sql statement updated onGenerateThreadResponseAnswer(data.updateThreadResponse.id); }, + onError: (error) => { + message.error('Failed to update SQL statement: ' + error.message); + }, });wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (2)
25-27: Enhance human-readable error messages.
While the fallback message is helpful, it might be beneficial to display a more descriptive error message to guide the user on potential next steps.
35-56: Validate openModal usage and error handling around user-supplied SQL.
Opening the “Fix SQL” modal witherror.invalidSqlis a great user flow. However, ensure that the modal’s submission or closure states handle cases whereerror.fixStatementmight reject or throw. Consider disabling the button while the fix is in progress to prevent duplicate calls.wren-ui/src/apollo/client/graphql/home.generated.ts (1)
101-102: Check final usage ofupdateThreadResponseresults.
AccessingupdateThreadResponsefields is straightforward. Ensure that the client handles any error or partial response from the server.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
wren-ui/src/apollo/client/graphql/__types__.ts(4 hunks)wren-ui/src/apollo/client/graphql/home.generated.ts(2 hunks)wren-ui/src/apollo/client/graphql/home.ts(1 hunks)wren-ui/src/apollo/server/resolvers.ts(1 hunks)wren-ui/src/apollo/server/resolvers/askingResolver.ts(2 hunks)wren-ui/src/apollo/server/schema.ts(2 hunks)wren-ui/src/apollo/server/services/askingService.ts(2 hunks)wren-ui/src/components/ErrorCollapse.tsx(2 hunks)wren-ui/src/components/modals/FixSQLModal.tsx(1 hunks)wren-ui/src/components/modals/QuestionSQLPairModal.tsx(2 hunks)wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/PreparationStatus.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/PreparationSteps.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/index.tsx(2 hunks)wren-ui/src/components/pages/home/preparation/step/FixedSQLFinished.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Organizing.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/ViewFinished.tsx(1 hunks)wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx(3 hunks)wren-ui/src/components/pages/home/promptThread/store.tsx(1 hunks)wren-ui/src/pages/home/[id].tsx(4 hunks)wren-ui/src/styles/components/button.less(1 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)wren-ui/src/utils/errorHandler.tsx(2 hunks)wren-ui/src/utils/helper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- wren-ui/src/components/pages/home/preparation/step/Organizing.tsx
- wren-ui/src/apollo/client/graphql/home.ts
- wren-ui/src/components/pages/home/preparation/step/ViewFinished.tsx
- wren-ui/src/components/pages/home/preparation/step/FixedSQLFinished.tsx
- wren-ui/src/components/ErrorCollapse.tsx
- wren-ui/src/styles/components/button.less
- wren-ui/src/apollo/server/resolvers.ts
- wren-ui/src/utils/helper.ts
- wren-ui/src/apollo/server/resolvers/askingResolver.ts
- wren-ui/src/components/pages/home/promptThread/store.tsx
- wren-ui/src/components/modals/QuestionSQLPairModal.tsx
- wren-ui/src/utils/errorHandler.tsx
- wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx
🧰 Additional context used
🧬 Code Definitions (6)
wren-ui/src/apollo/server/services/askingService.ts (2)
wren-ui/src/apollo/client/graphql/__types__.ts (2)
ThreadResponse(1199-1210)Error(433-439)wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1)
ThreadResponse(42-52)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (3)
wren-ui/src/apollo/client/graphql/__types__.ts (1)
Error(433-439)wren-ui/src/components/ErrorCollapse.tsx (1)
ErrorCollapse(33-56)wren-ui/src/components/modals/FixSQLModal.tsx (1)
FixSQLModal(16-135)
wren-ui/src/pages/home/[id].tsx (3)
wren-ui/src/apollo/server/services/askingService.ts (1)
updateThreadResponse(718-732)wren-ui/src/apollo/server/resolvers/askingResolver.ts (1)
updateThreadResponse(422-431)wren-ui/src/apollo/client/graphql/home.generated.ts (1)
useUpdateThreadResponseMutation(719-722)
wren-ui/src/apollo/client/graphql/__types__.ts (1)
wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1)
ThreadResponse(42-52)
wren-ui/src/components/pages/home/preparation/PreparationSteps.tsx (3)
wren-ui/src/components/pages/home/preparation/index.tsx (1)
Props(15-19)wren-ui/src/components/pages/home/preparation/step/FixedSQLFinished.tsx (1)
FixedSQLFinished(3-15)wren-ui/src/components/pages/home/preparation/step/ViewFinished.tsx (1)
ViewFinished(3-12)
wren-ui/src/apollo/client/graphql/home.generated.ts (1)
wren-ui/src/apollo/client/graphql/__types__.ts (3)
Exact(4-4)ThreadResponseUniqueWhereInput(1251-1253)UpdateThreadResponseInput(1342-1344)
🪛 Biome (1.9.4)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx
[error] 7-7: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (31)
wren-ui/src/components/pages/home/preparation/PreparationStatus.tsx (4)
1-9: Imports look good.All required modules and icons are correctly imported. There are no apparent issues here.
10-16: Clear separation of props and state initialization.The destructured props and local loading states are concise and easy to follow. Excellent job abstracting
askingTaskfromdata.
17-27: Well-structured onCancel and onReRun callbacks.Using a higher-order function
attachLoadingto manage loading states is neat. The calls tostopAskingTaskandreRunAskingTaskare correctly parameterized.
62-63: Graceful fallback to null.Returning
nullin the final condition is typical for conditional rendering of a React component.wren-ui/src/components/pages/home/preparation/index.tsx (5)
4-8: Modular imports for new components.Importing
Typography,Collapse,PreparationStatus, andPreparationStepsfrom separate modules indicates a well-organized structure.
15-19: Updated Props type is consistent with usage.Introducing
isAnswerFinishedas an optional prop aligns with the logic for toggling the collapse state. It’s straightforward and improves clarity on the completion state of an answer.
22-30: Effective control of the collapse panel via Component State.
isActiveis conditionally set based on whether SQL exists and later updated viaisAnswerFinished. This approach keeps the UI in sync with the query status.
32-39: Neat handling of error object creation.Wrapping the error object with additional props (
invalidSql,fixStatement) is a minimal, yet effective way to pass relevant data toErrorBoundary.
81-82: Use ofPreparationStepswithin a collapsible panel.Extracting step-related UI to
PreparationStepskeeps this component cleaner and follows good separation of concerns.wren-ui/src/components/pages/home/preparation/PreparationSteps.tsx (4)
1-29: Imports and styled-components usage look correct.All relevant sub-components and styling tools are declared.
StyledBadgeis well-defined for indicating process states.
30-47: Process state constants are well-managed.Fetching the next states for
SEARCHING,PLANNING, andGENERATINGfrom the state machine centralizes logic and maintains clarity.
119-129: FixedSQLTimelineSteps are concise and clear.Immediately returning a single timeline item for the fixed SQL state prevents code duplication in the main timeline logic.
131-141: ViewTimelineSteps unify the final state for pre-saved views.The single timeline item for
ViewFinishedencapsulates the final step for retrieving from a saved view in a neat, readable manner.wren-ui/src/utils/error/dictionary.ts (1)
122-126: Well-structured error message additionThe new error message for SQL statement validation in the Fix SQL flow follows the established pattern of other error messages in this dictionary. It uses consistent wording and structure.
wren-ui/src/apollo/server/schema.ts (2)
640-646: Well-defined input types for the update operationThe new input types follow GraphQL best practices with clear naming and appropriate field types. The
ThreadResponseUniqueWhereInputproperly identifies the entity to update, whileUpdateThreadResponseInputcontains the updatable fields.
1045-1048: Good mutation definition with appropriate arguments and return typeThe
updateThreadResponsemutation follows the established pattern for update operations with separatewhereanddataarguments. The return type ensures we get the updated entity after mutation.wren-ui/src/apollo/server/services/askingService.ts (1)
135-138: Clean interface definition for the new update methodThe interface method is well-defined with appropriate parameter and return types, maintaining consistency with other methods in the interface.
wren-ui/src/pages/home/[id].tsx (3)
33-33: Good import of the new mutation hookProperly imported the generated mutation hook to be used within the component.
181-185: Good implementation of the SQL update functionThe function is concise and properly leverages the mutation hook with the correct variable structure.
320-320: Clean integration of the new function into the provider valueThe new function is properly added to the provider value object, making it accessible to all components that consume this context.
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (2)
11-14: EnsurefixStatementis always defined or handle when undefined.
BecausefixStatementis optional, you should handle the case where it’s not provided to avoid runtime errors.Would you like a verification script that searches for all usages of
error.fixStatementto confirm they are guarded?
20-20:Details
✅ Verification successful
Validate the logic for detecting invalid SQL.
ThehasInvalidSqlflag is set based onerror.invalidSql. Ensure that any upstream assignment toerror.invalidSqlis consistent and fails gracefully if it’s missing.
🏁 Script executed:
#!/bin/bash # Description: Check all references to `invalidSql` in the codebase to ensure correct assignment & usage rg 'invalidSql'Length of output: 23439
Invalid SQL Detection Logic Verified
After reviewing both the code and upstream assignments, the use of!!error.invalidSqlinErrorBoundary.tsxhas been validated. Upstream,invalidSqlis consistently assigned (e.g., inpreparation/index.tsxandPreparationSteps.tsx), and its optional nature (as indicated in type definitions and GraphQL fragments) is properly handled by this check. The double-negation safely converts any falsy value (such asnull,undefined, or an empty string) intofalse, ensuring the logic fails gracefully wheninvalidSqlis missing.No changes are needed.
wren-ui/src/components/modals/FixSQLModal.tsx (3)
33-38: Add error handling or wrap this mutation call in a try/catch.
This closely mirrors a past suggestion to handle potential failures gracefully:const validateSql = async () => { const sql = form.getFieldValue('sql'); + try { await previewSqlMutation({ variables: { data: { sql, limit: 1, dryRun: true } }, }); + } catch (error) { + console.error('Validation error:', error); + throw error; + } };
59-68: Improve error handling in submit function.
A similar request was made previously. It’s recommended to parse and handle the error rather than only logging it, which can enhance the user experience.form .validateFields() .then(async (values) => { await validateSql(); await onSubmit(values.sql); onClose(); }) - .catch(console.error); + .catch((error) => { + console.error('Submit error:', error); + // Optionally provide user-facing feedback here + });
70-70: Confirm preview data rendering logic.
const showPreview = previewSqlResult.data || previewSqlResult.loading;might show an empty or partial state if the user triggers multiple previews rapidly. Verify concurrency scenarios and consider using React states or a queue to handle multiple requests.wren-ui/src/apollo/client/graphql/__types__.ts (4)
603-603: Introduction ofupdateThreadResponsemutation.
This new mutation expands your GraphQL schema to handle updates to thread responses. The approach appears correct. Ensure you have server-side checks or validations for necessary fields.
869-872: ReviewupdateThreadResponseargument structure.
The design ofwhere+datafollows standard GraphQL patterns. Just confirm that theThreadResponseUniqueWhereInputcovers all unique scenarios (e.g., if ID is enough to guarantee uniqueness).
1251-1253: Confirm ID-based identification sufficiency.
ThreadResponseUniqueWhereInputrelies solely on an integerid. Ensure this aligns with your DB schema so that the ID is guaranteed unique.
1342-1344: Optionalsqlfield inUpdateThreadResponseInput.
This allows partial updates. Confirm whether you plan to allow empty string or null to remove existing SQL.wren-ui/src/apollo/client/graphql/home.generated.ts (2)
95-99: Well-structured variable types for updating a thread response.
UpdateThreadResponseMutationVariableswithwhere+datameets best practices for GraphQL. No concerns here.
692-726:useUpdateThreadResponseMutationintegration looks solid.
The generated hook is consistent with typical Apollo patterns. Validate the UI usage to confirm you handle success/error states, loading spinners, etc.
| import useModalAction from '@/hooks/useModalAction'; | ||
| import ErrorCollapse from '@/components/ErrorCollapse'; | ||
| import { FixSQLModal } from '@/components/modals/FixSQLModal'; | ||
| import { Error } from '@/apollo/client/graphql/__types__'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename imported 'Error' to prevent overshadowing the global Error object.
This import name can cause confusion since “Error” is a built-in global object in JavaScript.
- import { Error } from '@/apollo/client/graphql/__types__';
+ import { Error as GqlError } from '@/apollo/client/graphql/__types__';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Error } from '@/apollo/client/graphql/__types__'; | |
| import { Error as GqlError } from '@/apollo/client/graphql/__types__'; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
@andreashimin lgtm, thanks! |
onlyjackfrost
left a comment
onlyjackfrost
left a comment
There was a problem hiding this comment.
LGTM for the backend part
4 participants
Description
This PR introduces a "Fix SQL" flow that allows users to manually provide a corrected SQL query when the AI-generated SQL fails or encounters an error.
Tasks
Screeshots
Remarks
Check with @wwwy3y3 if its ok
Summary by CodeRabbit
New Features
Improvements