feat(wren-ui): Reasoning UI#1414
Conversation
WalkthroughThis pull request introduces extensive changes across the codebase. GraphQL type definitions are updated to include additional fields and a new mutation for rerunning asking tasks. Components throughout the UI have been refactored or newly added—ranging from loading spinners and preparation steps to prompt handling and answer results. Several hooks and utility functions have been introduced or updated for state management, process state transitions, and global configuration. Telemetry setup is removed from the app component and replaced with a global configuration provider. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
andreashimin
force-pushed
the
feat/reasoning-ui
branch
from
eac9101 to
2acc011
Compare
wwwy3y3
force-pushed
the
feature/reasoning-api
branch
from
7f5a5d1 to
6c73ee8
Compare
andreashimin
force-pushed
the
feat/reasoning-ui
branch
2 times, most recently
from
40701e3 to
29bcff2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
wren-ui/src/apollo/server/services/askingService.ts (1)
377-443: 🛠️ Refactor suggestionShifting to
IAskingTaskTracker
IntegratingaskingTaskTrackercentralizes task operations, which improves maintainability. Double-check that concurrency (multiple tasks per thread) is handled gracefully.
♻️ Duplicate comments (1)
wren-ui/src/components/pages/home/preparation/index.tsx (1)
102-105:⚠️ Potential issueEnhance error handling for task request failures.
The component should handle cases where the task request has failed while continuing to poll for instant recommended questions.
Add error handling to catch and display appropriate messages when the task fails but polling continues:
const processState = useMemo( () => convertAskingTaskToProcessState(askingTask), [askingTask], ); + + const hasError = useMemo( + () => askingTask.status === AskingTaskStatus.FAILED, + [askingTask.status] + );Then update the UI to display an error message when
hasErroris true.
🧹 Nitpick comments (34)
wren-ui/src/utils/enum/home.ts (1)
13-14: Enhanced process state tracking with new statesThe addition of
CORRECTINGandSTOPPEDstates to the PROCESS_STATE enum appropriately extends the tracking capabilities for the asking task flow. These new states will help provide better visibility into task progress and status.Consider adding comments to document when these states are expected to occur in the process flow, which would enhance maintainability for future developers.
export enum PROCESS_STATE { IDLE, UNDERSTANDING, SEARCHING, PLANNING, GENERATING, - CORRECTING, + CORRECTING, // When the system is correcting errors in the generated output FINISHED, FAILED, - STOPPED, + STOPPED, // When the user manually stops the task execution NO_RESULT, }Also applies to: 16-16
wren-ui/src/components/pages/home/preparation/step/Generating.tsx (1)
10-37: Well-structured component with clear state visualization.The component effectively communicates the SQL generation process state to users through appropriate messaging and loading indicators. The conditional rendering logic is clean and easy to understand.
One minor suggestion: consider adding aria attributes for improved accessibility:
- <Spinner className="gray-6" size={12} /> + <Spinner className="gray-6" size={12} aria-label="Generating SQL statement" /> - <Spinner className="gray-6" size={16} /> + <Spinner className="gray-6" size={16} aria-label="Wrapping up" />wren-ui/src/utils/telemetry.ts (1)
5-29: Well-implemented PostHog initialization with proper browser-side check.The PostHog setup function correctly handles Next.js SSR by checking for client-side execution and configures the telemetry service with appropriate settings. The PostHog configuration is comprehensive, with careful consideration of privacy concerns (masked inputs) and development environment handling.
Consider storing the debug message behind a debug flag rather than always logging to console:
- console.log('PostHog initialized.'); + if (env.isDevelopment) { + console.log('PostHog initialized.'); + }wren-ui/src/hooks/useStoreContext.tsx (1)
1-33: Good context management patternThe
useStoreContexthook provides a clean mechanism for dynamically creating and managing React contexts. This will help with state management across the refactored query flow components.Consider adding TypeScript generics to the hook to make it more type-safe:
- const createStore = (id: string) => { + const createStore = <T,>(id: string) => { - if (contextMap.has(id)) return contextMap.get(id); + if (contextMap.has(id)) return contextMap.get(id) as React.Context<T>; - const context = createContext(null); + const context = createContext<T | null>(null); contextMap.set(id, context); - return context; + return context; }; - const useStore = (id: string) => { + const useStore = <T,>(id: string) => { const context = contextMap.get(id); if (!context) throw new Error(`Context not found for id: ${id}`); - return useContext(context); + return useContext(context) as T; };wren-ui/src/components/pages/home/preparation/step/Organizing.tsx (2)
17-31: Consider memoizing the scroll functionThe scroll logic is well-implemented with appropriate effects that trigger based on different conditions. Consider memoizing the
scrollBottomfunction withuseCallbackto prevent unnecessary re-creation on renders.- const scrollBottom = () => { + const scrollBottom = useCallback(() => { if ($wrapper.current) { $wrapper.current.scrollTo({ top: $wrapper.current.scrollHeight, }); } - }; + }, []);
33-52: Consider more responsive height constraintsThe component's UI is well-structured with conditional rendering based on loading state. However, the hardcoded max height
calc(100vh - 550px)might not be flexible across different screen sizes. Consider making this value more responsive or configurable via props.<div ref={$wrapper} className="gray-7 text-sm mt-2" - style={{ maxHeight: 'calc(100vh - 550px)', overflowY: 'auto' }} + style={{ maxHeight: 'calc(100vh - 550px)', overflowY: 'auto', minHeight: '100px' }}Alternatively, consider making it configurable through props:
interface Props { stream: string; loading?: boolean; + maxHeight?: string; } // Then use it in the style style={{ maxHeight: props.maxHeight || 'calc(100vh - 550px)', overflowY: 'auto' }}wren-ui/src/components/PageLoading.tsx (1)
39-41: New Spinner component provides a simplified loading indicatorThe new
Spinnercomponent offers a streamlined version of the loading indicator, which is useful for inline or smaller loading states. It effectively uses theclassNameandsizeprops for customization.Note that while the
LoadingPropsinterface was extended withwidthandheightproperties, these aren't used in this component. Consider either utilizing these properties or documenting which components use which properties.wren-ui/src/components/pages/home/preparation/step/Retrieving.tsx (1)
27-37: Add handling for empty tables arrayThe component doesn't handle the case where the
tablesarray is empty. Consider adding a conditional check to display an appropriate message when no tables are found.{loading ? ( <div className="d-flex align-center gx-2"> Searching <Spinner className="gray-6" size={12} /> </div> ) : ( <> - <div className="mb-1">3 tables found</div> - <TagIterator data={data} /> + {tables.length > 0 ? ( + <> + <div className="mb-1">{tables.length} tables found</div> + <TagIterator data={data} /> + </> + ) : ( + <div className="mb-1">No tables found</div> + )} </> )}wren-ui/src/hooks/useGlobalConfig.tsx (1)
12-33: Add proper TypeScript typing for the children propThe
GlobalConfigProvidercomponent is missing proper TypeScript typing for thechildrenprop. You should specify the React.ReactNode type to ensure type safety.-export const GlobalConfigProvider = ({ children }) => { +export const GlobalConfigProvider = ({ children }: { children: React.ReactNode }) => {wren-ui/src/components/pages/home/promptThread/store.tsx (1)
60-60: Add type safety check before castingThe current type cast to
IPromptThreadStoredoesn't have any safety checks. If the store doesn't exist, this could lead to runtime errors.- return storeContext.useStore(STORE.PROMPT_THREAD) as IPromptThreadStore; + const store = storeContext.useStore(STORE.PROMPT_THREAD); + if (!store) { + throw new Error('PromptThreadStore must be used within a PromptThreadProvider'); + } + return store as IPromptThreadStore;wren-ui/src/components/pages/home/prompt/Input.tsx (2)
33-35: Add proper type for the event parameterThe
syncInputValuefunction has an untyped event parameter. You should add proper React typing for better type safety.- const syncInputValue = (event) => { + const syncInputValue = (event: React.ChangeEvent<HTMLTextAreaElement>) => { setInputValue(event.target.value); };
52-63: Enhance accessibility with ARIA attributesThe TextArea could benefit from additional ARIA attributes to improve accessibility for users with disabilities.
ref={$promptInput} // disable grammarly data-gramm="false" size="large" autoSize placeholder="Ask to explore your data" value={inputValue} onInput={syncInputValue} onPressEnter={inputEnter} disabled={isDisabled} + aria-label="Data exploration query input" + role="textbox" + aria-multiline="true"wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (1)
128-129: Consider using a more consistent pattern for optional field handling.The current implementation works correctly, but consider using the nullish coalescing operator for consistency:
- sql: data.sql ? data.sql : undefined, - viewId: data.viewId ? data.viewId : undefined, + sql: data.sql ?? undefined, + viewId: data.viewId ?? undefined,This would handle both
nullandundefinedcases consistently and matches the pattern used in modern TypeScript codebases.wren-ui/src/apollo/server/schema.ts (1)
992-992: New mutation enables task rerunning.The
rerunAskingTaskmutation allows users to retry the reasoning process from a specific response, enhancing the interactive nature of the reasoning UI.Consider adding documentation comments to explain the purpose and behavior of this mutation, particularly how it differs from
createAskingTaskand what happens to existing responses when a task is rerun.wren-ui/src/components/pages/home/promptThread/ChartAnswer.tsx (1)
96-101: Error handling should use the ErrorHandler system.According to the retrieved learning from PR #1090, error handling for dashboard operations should be implemented through the ErrorHandler system instead of handling errors inline.
Replace the inline error handling with the ErrorHandler system:
const [createDashboardItem] = useCreateDashboardItemMutation({ - onError: (error) => console.error(error), + onError: (error) => ErrorHandler.handleError(error, 'Failed to pin chart to dashboard'), onCompleted: () => { message.success('Successfully pinned chart to dashboard.'); }, });wren-ui/src/components/pages/home/promptThread/index.tsx (1)
101-106: Improved scroll behavior logic.The effect that controls scrolling now uses specialized functions to determine when a response is complete, making the code more readable and maintainable.
Consider consolidating the
getIsFinishedandgetAnswerIsFinishedfunctions into a single utility or adding comments to clarify their different purposes, as having two separate "is finished" functions could be confusing to developers.wren-ui/src/hooks/useAskProcessState.tsx (1)
87-148:ProcessStateMachineclass with static members only.
- The static-only pattern works but may be simplified into standalone functions if preferred.
- Using
thisin a static context can sometimes be substituted with the class name for clarity.🧰 Tools
🪛 Biome (1.9.4)
[error] 87-148: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 130-130: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 137-137: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
wren-ui/src/apollo/server/repositories/askingTaskRepository.ts (1)
42-45: Improve error handling for non-plain objects.Currently, the code throws a generic
'Unexpected dbdata'error. Providing more context, such as the offending object's structure, could help with faster debugging.if (!isPlainObject(data)) { - throw new Error('Unexpected dbdata'); + throw new Error(`Unexpected dbdata: ${JSON.stringify(data)}`); }wren-ui/src/apollo/server/services/askingTaskTracker.ts (1)
219-222: Potential concurrency and load.Polling every second can cause unnecessary load if many tasks accumulate. Consider using a configurable scheduler with backoff or a job queue (e.g., Bull, Agenda) to scale more efficiently.
wren-ui/src/components/pages/home/prompt/Result.tsx (1)
158-160: Use optional chaining for the function call.Rather than
onIntentSQLAnswer && onIntentSQLAnswer(), you can simplify it with optional chaining. This is more idiomatic and addresses the analyzer hint.- if (type === AskingTaskType.TEXT_TO_SQL) { - onIntentSQLAnswer && onIntentSQLAnswer(); - } + if (type === AskingTaskType.TEXT_TO_SQL) { + onIntentSQLAnswer?.(); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
wren-ui/src/hooks/useAskPrompt.tsx (7)
39-45: Consider clarifying function names or adding docstrings.
ThecanGenerateAnswerandcanFetchThreadResponseutilities are helpful, but it might be beneficial to document the intended calling context or add inline comments for better readability.
47-49: Minor naming alignment
isReadyToThreadResponsemight be clearer asisReadyForThreadResponse, to read more naturally.-export const isReadyToThreadResponse = (askingTask: AskingTask) => +export const isReadyForThreadResponse = (askingTask: AskingTask) =>
60-69: Consider renaming for clarity
isNeedRecommendedQuestionsis slightly awkward syntactically. A name likeneedsRecommendedQuestionsorshouldFetchRecommendedQuestionsmight be more natural.
74-110: Safe concurrency handling in cache update
handleUpdateThreadCachedirectly updates the cache based on thequeryId. This is valid, but be mindful of concurrent updates if multiple tasks for the same thread run in parallel. Review whether more robust concurrency controls might be needed.
211-218: Verify early return usage
checkFetchAskingStreamTaskreturns early ifaskingStreamTaskis already set. Ensure we don’t need to refresh or override an existing stream when a new task is triggered.
250-259: Graceful cancellation
UsingnextTick(1000)ensures polling stops before continuing. If cancellations become more frequent, consider a more direct approach to confirm the task is fully canceled, rather than a fixed delay.
261-281: Robust error handling
onReRuntries to rerun the task and update the cache. Make sure no partial updates happen if the rerun fails halfway. Logging is good; additional UI feedback on error might be helpful.wren-ui/src/apollo/server/services/askingService.ts (3)
118-119: Return type changed toTrackedAskingResult | null
This is more robust, but confirm any calling code checks fornullproperly to avoid runtime errors.
567-598: Rerun logic
Properly reusescreateAskingTask. If any advanced merging or partial reusability of prior results is needed, you might add a more detailed strategy in the future.
1001-1024:getAskingHistory
Filtering out the canceled thread response is a good improvement. Check for performance if the thread has many historical responses. Possibly limit further if needed.wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx (3)
150-159:isNeedGenerateAnswer
Function has a straightforward approach: it checks if the background task is not started and the answer is not finished. The naming is slightly off grammatically, but logically it works.-const isNeedGenerateAnswer = (answerDetail: ThreadResponseAnswerDetail) => { +const needsAnswerGeneration = (answerDetail: ThreadResponseAnswerDetail) => {
161-175: Refined store usage
Use ofusePromptThreadStore()to unify new logic is a net improvement. Clarify any potential race conditions if multiple answer details get updated rapidly.
262-264:TextBasedAnswer
Seamless that you pass the entirepropsobject. Just ensure performance is not impacted by rerenders whenpropschanges.wren-ui/src/apollo/client/graphql/__types__.ts (1)
742-744:MutationRerunAskingTaskArgsforresponseId
This new argument structure is simple and self-explanatory. Consider whether you need extra parameters if rerun logic might vary by context. Otherwise, looks good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-ui/public/images/icon/message-ai.svgis excluded by!**/*.svg
📒 Files selected for processing (44)
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/client/graphql/__types__.ts(6 hunks)wren-ui/src/apollo/client/graphql/home.generated.ts(10 hunks)wren-ui/src/apollo/client/graphql/home.ts(4 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.ts(1 hunks)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)wren-ui/src/apollo/server/services/askingTaskTracker.ts(1 hunks)wren-ui/src/apollo/server/services/index.ts(1 hunks)wren-ui/src/apollo/server/telemetry/telemetry.ts(1 hunks)wren-ui/src/apollo/server/utils/error.ts(3 hunks)wren-ui/src/common.ts(7 hunks)wren-ui/src/components/PageLoading.tsx(1 hunks)wren-ui/src/components/learning/index.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/index.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Generating.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Organizing.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Retrieving.tsx(1 hunks)wren-ui/src/components/pages/home/prompt/Input.tsx(1 hunks)wren-ui/src/components/pages/home/prompt/Result.tsx(10 hunks)wren-ui/src/components/pages/home/prompt/index.tsx(4 hunks)wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx(3 hunks)wren-ui/src/components/pages/home/promptThread/BreakdownAnswer.tsx(2 hunks)wren-ui/src/components/pages/home/promptThread/ChartAnswer.tsx(4 hunks)wren-ui/src/components/pages/home/promptThread/TextBasedAnswer.tsx(3 hunks)wren-ui/src/components/pages/home/promptThread/index.tsx(4 hunks)wren-ui/src/components/pages/home/promptThread/store.tsx(1 hunks)wren-ui/src/hooks/useAskProcessState.tsx(2 hunks)wren-ui/src/hooks/useAskPrompt.tsx(8 hunks)wren-ui/src/hooks/useGlobalConfig.tsx(1 hunks)wren-ui/src/hooks/useStoreContext.tsx(1 hunks)wren-ui/src/pages/_app.tsx(2 hunks)wren-ui/src/pages/home/[id].tsx(5 hunks)wren-ui/src/pages/home/index.tsx(2 hunks)wren-ui/src/utils/enum/home.ts(1 hunks)wren-ui/src/utils/env.ts(1 hunks)wren-ui/src/utils/telemetry.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
wren-ui/src/components/pages/home/promptThread/ChartAnswer.tsx (1)
Learnt from: andreashimin
PR: Canner/WrenAI#1090
File: wren-ui/src/components/pages/home/promptThread/ChartAnswer.tsx:107-112
Timestamp: 2025-03-13T01:43:25.329Z
Learning: Error handling for dashboard operations should be implemented through the ErrorHandler system instead of handling errors inline. This ensures consistent error handling across the application.
wren-ui/src/apollo/server/services/askingService.ts (1)
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.
🧬 Code Definitions (11)
wren-ui/src/components/pages/home/preparation/step/Organizing.tsx (1)
wren-ui/src/components/PageLoading.tsx (1) (1)
Spinner(39:41)
wren-ui/src/pages/_app.tsx (1)
wren-ui/src/hooks/useGlobalConfig.tsx (1) (1)
GlobalConfigProvider(12:33)
wren-ui/src/utils/telemetry.ts (1)
wren-ui/src/utils/env.ts (1) (1)
UserConfig(8:13)
wren-ui/src/common.ts (2)
wren-ui/src/apollo/server/repositories/askingTaskRepository.ts (1) (1)
AskingTaskRepository(27:71)wren-ui/src/apollo/server/services/askingTaskTracker.ts (1) (1)
AskingTaskTracker(56:471)
wren-ui/src/components/pages/home/promptThread/ChartAnswer.tsx (1)
wren-ui/src/components/pages/home/promptThread/store.tsx (1) (1)
usePromptThreadStore(58:61)
wren-ui/src/hooks/useAskProcessState.tsx (1)
wren-ui/src/apollo/client/graphql/__types__.ts (1) (1)
AskingTask(26:39)
wren-ui/src/components/pages/home/promptThread/TextBasedAnswer.tsx (1)
wren-ui/src/components/pages/home/promptThread/store.tsx (1) (1)
usePromptThreadStore(58:61)
wren-ui/src/hooks/useGlobalConfig.tsx (2)
wren-ui/src/utils/env.ts (2) (2)
UserConfig(8:13)getUserConfig(16:23)wren-ui/src/utils/telemetry.ts (1) (1)
trackUserTelemetry(31:45)
wren-ui/src/apollo/server/services/askingService.ts (4)
wren-ui/src/apollo/server/services/askingTaskTracker.ts (2) (2)
TrackedAskingResult(31:35)IAskingTaskTracker(43:54)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)
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)
wren-ui/src/components/pages/home/prompt/index.tsx (2)
wren-ui/src/hooks/useAskProcessState.tsx (1) (1)
getIsProcessing(9:16)wren-ui/src/components/pages/home/prompt/Input.tsx (1) (1)
PromptInput(16:75)
🪛 Biome (1.9.4)
wren-ui/src/hooks/useAskProcessState.tsx
[error] 87-148: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 130-130: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 137-137: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
wren-ui/src/components/pages/home/prompt/Result.tsx
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
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)
wren-ui/src/components/pages/home/prompt/index.tsx
[error] 125-125: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (176)
wren-ui/src/common.ts (7)
18-18: Proper integration of AskingTaskRepositoryThe addition of
AskingTaskRepositoryto the imports list properly prepares for its instantiation and usage within the components initialization function.
33-33: Proper import of AskingTaskTracker serviceThe AskingTaskTracker import has been correctly added to the services import section, maintaining consistent code organization.
68-68: Correct initialization of askingTaskRepositoryThe repository is properly instantiated with the knex connection, maintaining the pattern used for other repositories.
111-116: Well-structured initialization of AskingTaskTrackerThe AskingTaskTracker is properly instantiated with all its required dependencies. The dependency injection approach follows the existing pattern and promotes testability.
127-127: Proper integration with AskingServiceThe askingTaskTracker is correctly passed to the AskingService constructor, allowing the service to utilize the new tracking functionality.
171-172: Correct export of askingTaskRepositoryThe askingTaskRepository is properly included in the returned components, following the established pattern for exposing repositories.
188-188: Correct export of askingTaskTrackerThe askingTaskTracker is properly exposed in the returned components, making it available for use throughout the application.
wren-ui/src/apollo/server/services/index.ts (1)
9-9: Proper export of askingTaskTracker moduleThe new export statement correctly follows the existing pattern in this file, making the askingTaskTracker module available for import elsewhere in the application.
wren-ui/src/apollo/server/repositories/index.ts (1)
18-18: Proper export of askingTaskRepository moduleThe new export statement correctly follows the existing pattern in this file, making the askingTaskRepository module available for import elsewhere in the application.
wren-ui/src/utils/env.ts (1)
8-8: Type export provides better reusabilityExporting the
UserConfigtype makes it accessible from other modules, which allows for better type checking across the application and supports the refactored user query flow mentioned in the PR objectives.wren-ui/src/apollo/server/telemetry/telemetry.ts (1)
54-54: Telemetry event added for new rerun task functionalityThis new telemetry event aligns with the PR objectives of refactoring the user query flow, allowing tracking when users rerun asking tasks. The naming convention follows the established pattern and clearly indicates the context and action being tracked.
wren-ui/src/apollo/server/resolvers.ts (1)
94-94:Details
✅ Verification successful
New mutation for rerunning asking tasks
The addition of the
rerunAskingTaskresolver function to the Mutations object provides the GraphQL API endpoint needed for the new task rerunning functionality. This directly supports the PR objective of refactoring the user query flow and enhancing the interaction experience.
🏁 Script executed:
#!/bin/bash # Verify that the corresponding resolver function exists in askingResolver.ts echo "Checking for rerunAskingTask implementation in askingResolver.ts..." rg "rerunAskingTask" --type ts -A 5 -B 5 "wren-ui/src/apollo/server/resolvers/askingResolver.ts" # Verify that the mutation is also defined in the GraphQL schema echo "Checking for rerunAskingTask mutation in the GraphQL schema..." rg "rerunAskingTask" --type ts "wren-ui/src/apollo/server/schema.ts"Length of output: 1533
Verified & Approved: New Mutation for Rerunning Asking Tasks
- Confirmed that the
rerunAskingTaskresolver is implemented inwren-ui/src/apollo/server/resolvers/askingResolver.ts.- The GraphQL schema in
wren-ui/src/apollo/server/schema.tscorrectly declares thererunAskingTask(responseId: Int!): Task!mutation.- These changes successfully support the PR objective of refactoring the user query flow and enhancing the interaction experience.
wren-ui/migrations/20250509000001_add_task_id_to_thread.js (3)
1-14: Database migration adds task ID relationship to thread responsesThis migration creates a relationship between thread responses and asking tasks by adding a foreign key, which is necessary for the refactored query flow. The implementation correctly handles the case where asking tasks are deleted by setting the reference to null rather than cascading deletes.
16-25: Proper rollback function to remove the foreign keyThe down migration correctly removes both the foreign key constraint and the column, ensuring clean rollbacks if needed.
1-26:Details
❓ Verification inconclusive
Verify migration date in filename
The migration filename contains what appears to be a future date (2025-05-09). While this won't affect functionality, it's worth confirming if this naming convention is intentional for your migration versioning.
🏁 Script executed:
#!/bin/bash # Check the pattern of other migration filenames in the project echo "Listing other migration files for comparison:" fd "^\d+.*\.js$" wren-ui/migrations/ --exec ls -la {}Length of output: 4124
Migration Timestamp Confirmation
After reviewing the migration filenames in the project, it appears that using a 2025-05-09 timestamp is consistent with the current naming convention (see, for example, the companion file
20250509000000_create_asking_task.js). This indicates that the future-dated timestamp is likely an intentional part of your migration ordering strategy. Please confirm that this naming approach is deliberate.wren-ui/src/apollo/server/models/adaptor.ts (1)
125-130: Expanded properties enhance the AskResult type with detailed response information.The addition of these optional properties (
rephrasedQuestion,sqlGenerationReasoning,retrievedTables,invalidSql, andtraceId) provides more comprehensive information about the SQL generation process, which aligns well with the PR objective of refactoring the user query flow. These fields will likely enable improved feedback and transparency in the Text-to-SQL feature.wren-ui/src/components/pages/home/preparation/step/Generating.tsx (1)
4-8: Clean interface definition for the component props.The props interface is well-defined with clear, descriptive boolean properties that represent different states of the SQL generation process.
wren-ui/src/components/learning/index.tsx (1)
254-255: Improved null safety when setting transition style.Good defensive programming change. This update prevents potential runtime errors by checking if
$collapseBlock.currentexists before attempting to set its style property.wren-ui/src/utils/telemetry.ts (1)
31-45: Clean implementation of page view tracking with proper cleanup.The tracking implementation properly handles the setup and teardown of event listeners, preventing potential memory leaks. The conditional initialization based on user preferences respects user privacy choices.
wren-ui/src/pages/_app.tsx (1)
6-6: Clean refactoring with GlobalConfigProviderThe introduction of
GlobalConfigProviderand the restructuring of the component hierarchy is a good improvement. This change centralizes the configuration management and separates the telemetry setup logic from the App component.Also applies to: 22-30
wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts (1)
643-653: Enhanced result data structureThe additional fields enriching the AskResult object (rephrasedQuestion, intentReasoning, etc.) provide more comprehensive information about the query processing, which aligns with the PR's goal of improving the Text-to-SQL user query flow.
wren-ui/migrations/20250509000000_create_asking_task.js (1)
1-34: Well-structured database migrationThe migration creates a new
asking_tasktable with appropriate fields and relationships. The schema is properly designed with:
- Unique constraints on query_id
- Foreign key relationships with appropriate cascade behavior
- Default values where needed
- Automatic timestamp management
This table structure will effectively support the enhanced query flow tracking.
wren-ui/src/components/pages/home/preparation/step/Organizing.tsx (1)
1-16: Well-structured component with clear prop interfacesThe component is well-organized with appropriate imports and a clear interface definition. The
isDoneflag is a good pattern to derive state from props.wren-ui/src/components/PageLoading.tsx (1)
29-32: Interface extension enhances component flexibilityAdding these optional properties to the
LoadingPropsinterface enables more customization options for loading indicators throughout the application.wren-ui/src/components/pages/home/promptThread/BreakdownAnswer.tsx (2)
14-15: Simplified props signature improves code clarityAccepting the full
AnswerResultPropstype directly instead of usingPicksimplifies the component interface and makes it more maintainable. Removing the unusedmotionproperty also cleans up the code.
42-57: Simplified className structureRemoving the conditional class application (likely from
clsx) in favor of a direct className string improves code readability and simplifies the component structure without changing functionality.wren-ui/src/components/pages/home/preparation/step/Retrieving.tsx (3)
5-8: Well-defined props interfaceThe interface is clear and appropriately defines the expected props for this component.
10-15: Good separation of concerns with TagTemplate and TagIteratorUsing a separate component for tag rendering and leveraging the
makeIterableutility is a clean approach for rendering lists of items.
24-24: Use "models" instead of "tables"A previous review comment suggested using "models" instead of "tables" for consistency with the rest of the codebase.
wren-ui/src/apollo/client/graphql/home.ts (4)
58-84: Well-structured GraphQL fragmentThe new
COMMON_ASKING_TASKfragment is well-structured and follows the existing patterns in the codebase. It properly includes all necessary fields and reuses theCOMMON_ERRORfragment consistently.
107-109: Good modularization of GraphQL fragmentsThe update to use the
COMMON_ASKING_TASKfragment in theCOMMON_RESPONSEfragment is a good improvement for code modularity and maintainability. The proper import of the fragment at line 115 ensures consistency.Also applies to: 115-115
146-152: Good refactoring using fragmentThe update to the
ASKING_TASKquery to use theCOMMON_ASKING_TASKfragment instead of listing all fields explicitly is a good improvement that enhances maintainability and consistency.
198-204:Details
✅ Verification successful
Verify integration of the new RERUN_ASKING_TASK mutation
The new mutation looks good, but we should verify that it's properly integrated with the backend resolver and the client-side code that will use it.
🏁 Script executed:
#!/bin/bash # Check if the rerunAskingTask mutation is properly implemented in the resolver # Look for the resolver implementation rg -A 5 "rerunAskingTask.*\(.*\)" --type tsLength of output: 2408
Integration Verified: RERUN_ASKING_TASK mutation works as expected
Verification confirms that:
- The mutation is correctly defined in wren-ui/src/apollo/client/graphql/home.ts and properly propagated to the generated file.
- The GraphQL schema in wren-ui/src/apollo/server/schema.ts includes the expected signature (
rerunAskingTask(responseId: Int!): Task!).- The corresponding resolver in wren-ui/src/apollo/server/resolvers/askingResolver.ts binds the mutation method.
- The client-side integration, as seen in wren-ui/src/hooks/useAskPrompt.tsx, correctly uses the
useRerunAskingTaskMutationhook.No changes are needed.
wren-ui/src/components/pages/home/promptThread/TextBasedAnswer.tsx (3)
8-8: Good addition of store integration.Adding the import for
usePromptThreadStoreenables the component to leverage centralized state management, improving code organization.
36-38: Simplified props interface improves component flexibility.Changing from
Pick<AnswerResultProps, | 'threadResponse' | 'isLastThreadResponse' | 'onInitPreviewDone' | 'onRegenerateTextBasedAnswer'>toAnswerResultPropsmakes the component more maintainable and reduces the need to update the prop interface when new properties are added.
109-110: Improved state management with store function.Replacing the prop callback with a store function follows better state management practices and centralizes the regeneration logic.
wren-ui/src/pages/home/index.tsx (4)
115-115: Simplified question submission.Removing the
nextTickcall streamlines the function as it was likely unnecessary. The function now directly submits the question to the prompt.
118-126: Function renaming improves clarity of purpose.Renaming
onSelecttoonCreateResponsebetter reflects the function's purpose. The implementation is also simplified by removing the recommendation questions generation logic.
139-141: Updated props for RecommendedQuestionsInstruction component.The props have been correctly updated to reflect the renamed function and simplified loading state.
143-147: Updated props for Prompt component.The Prompt component now correctly receives the renamed
onCreateResponsefunction.wren-ui/src/components/pages/home/preparation/index.tsx (3)
43-96: Well-structured PreparationStatus component.The PreparationStatus component effectively manages different states of the preparation process and provides appropriate user controls (cancel/re-run) based on the current state.
111-113: Good use of useEffect for state management.The component correctly updates its active state based on whether the answer is prepared, ensuring the UI stays in sync with the application state.
177-208: Well-organized preparation steps with appropriate loading states.The timeline implementation clearly presents the different steps in the preparation process (retrieving, organizing, generating) with appropriate loading indicators.
wren-ui/src/apollo/server/repositories/threadResponseRepository.ts (3)
44-44: Database schema alignment with model interface.The addition of an optional
askingTaskIdproperty to theThreadResponseinterface properly connects this model to theasking_tasktable, supporting the new reasoning UI feature as mentioned in the PR objectives.
48-48: Making sql optional supports flexible response handling.Changing
sqlfrom required to optional allows the model to handle responses that might not yet have SQL generated, which is important for the step-by-step reasoning process.
118-119: Update method appropriately extended.Adding
sqlandviewIdas optional properties in the update method's data parameter aligns with the interface changes and enables more flexible updates.wren-ui/src/apollo/server/schema.ts (4)
587-593: Enhanced AskingTask type for better reasoning flow representation.The additional fields (
rephrasedQuestion,sqlGenerationReasoning,retrievedTables,invalidSql,traceId,queryId) provide valuable context for the reasoning process, making each step of the SQL generation more transparent.
623-623: Input types shifted from view-based to task-based approach.Changing
viewIdtotaskIdin both input types reflects the architectural shift toward a task-centered model for managing the query flow, which aligns with the PR objective of refactoring the user query flow.Also applies to: 629-629
704-704: ThreadResponse.sql field made optional.Making the
sqlfield nullable in the GraphQL schema matches the repository layer changes and provides flexibility in representing responses that don't have SQL yet.
709-709: Nullable AskingTask relationships provide flexibility.Making
askingTaskrelationships nullable in both the ThreadResponse type and the query return type improves API robustness by handling cases where tasks might not exist.Also applies to: 913-913
wren-ui/src/components/pages/home/promptThread/ChartAnswer.tsx (3)
13-14: Improved imports for better code organization.The imports have been updated to support the new store-based architecture, with proper typing from GraphQL types.
Also applies to: 23-23
80-82: Refactored to use centralized state management.The component now uses
usePromptThreadStoreto access chart-related actions instead of receiving them through props, reducing prop drilling and improving code maintainability.
160-160: Function naming aligned with store actions.The function name change from
onRegenerateChartAnswertoonGenerateChartAnswerstandardizes the API to use consistent naming across the codebase.wren-ui/src/components/pages/home/promptThread/index.tsx (3)
5-5: Updated imports to support new architecture.The imports now properly include the store and utility functions for checking task completion status.
Also applies to: 8-8
41-46: Simplified component props.The AnswerResultTemplate component now has a cleaner interface with reduced props, leveraging the store for most of its functionality.
69-74: Removed props in favor of store-based state.The PromptThread component now uses the store to access its data instead of receiving it through props, which simplifies the component and reduces prop drilling.
wren-ui/src/components/pages/home/prompt/index.tsx (15)
10-10: No issues with this import change.
22-22: Renaming the prop clarifies its intent.
35-35: Interface update appears consistent.
57-57: Prop rename toonCreateResponse.
Looks good; ensure all referencing components adhere to this updated naming for clarity.
81-84: Added state variables for tracking visibility and input.
The introduction ofshowResultandquestionstreamlines your prompt flow.
87-90: MemoizedisProcessingcomputation.
This leveragesgetIsProcessingto keep the rendering optimized. Good approach.
94-95: State transition onaskingTaskchange.
Ensures the process state is updated consistently. No concerns spotted.
106-111:selectRecommendedQuestionmethod.
This is a neat separation for creating a response from recommended questions. Nice structure.
115-119:intentSQLAnswerhandles text-to-sql response creation.
Implementation is clear; any failures are handled by your existing promise flow. Good job.
124-124: Clearing the question on close.
Helps reset the component state between sessions.
129-131:stopProcessmethod introduced.
Effectively cancels the operation and resets the prompt. Looks fine.
135-141:submitAskmanages submission flow.
Including a check forisProcessingprevents spamming multiple requests. Well-implemented.
155-158: Transition to usingPromptInput.
Encapsulates input logic nicely and matches the component refactor. Good approach.
160-160: Minor placeholder line change.
No functional change; safe to overlook.
161-172: Conditional rendering ofPromptResult.
This is straightforward, ensuring the result is only shown when applicable. Implementation looks correct.wren-ui/src/hooks/useAskProcessState.tsx (6)
3-7: Expanding imports to include new AskingTask types.
The updated imports align with the introduction of broader state handling. No concerns.
12-15: Added SEARCHING and CORRECTING to processing states.
Keeps parity with updated enumerations. Looks consistent.
18-40:convertAskingTaskToProcessStatefunction.
• Correctly returnsNO_RESULTif there are no candidates for TEXT_TO_SQL tasks.
• Mapping fromAskingTaskStatustoPROCESS_STATEis well-structured.
51-63:matchedStatevalidates transitions viaProcessStateMachine.
Strong approach to avoid invalid transitions. Good use of console warnings.
65-67:transitionTosets the new state.
Straightforward logic, no issues found.
69-75:isFinishedandisFailedhelpers.
These concise checks improve overall readability.wren-ui/src/pages/home/[id].tsx (14)
3-10: Extended React imports and hooks.
Bringing in necessary hooks is aligned with new functionality. No issue found.
25-25:QuestionSQLPairModalimport location changed.
No concerns. Organizational detail is minor.
28-28: ImportingPromptThreadProvider.
Enables broader context injection. Looks fine.
163-165: StoringresponsesandpollingResponseinuseMemo.
Prevents redundant computations. Useful for performance.
200-203:onGenerateThreadRecommendedQuestionsfunction added.
Asynchronously fetching recommendation questions is consistent with the rest of the flow.
205-230:handleUnfinishedTasksto manage incomplete states.
• Good approach to systematically detect both incomplete asking tasks and thread responses.
• Maintains clarity in the live polling mechanism.
234-239:storeQuestionsToAskPromptfor batch storing thread questions.
Centralizes logic for recommended questions. Nicely integrated withaskPrompt.
245-255: Cleanup logic in theuseEffectreturn.
Effectively stops polling and resets prompt upon thread change or unmount.
259-262: Triggering handling of unfinished tasks and storing questions.
Implements a coherent re-initialization step wheneverresponsesupdates.
265-269: Polling stops once the response is finished.
Prevents unnecessary polling and toggles recommendation visibility. Good practice.
284-289:onCreateResponsefunction to create new thread response.
The simplified approach is direct and consistent with the overall data flow.
298-315:providerValueobject providing multiple handlers and states.
Facilitates a well-structured context forPromptThread. Good design choice.
319-321: WrappingPromptThreadinPromptThreadProvider.
Ensures children have immediate access to shared values. Implementation is correct.
325-328: Ref passing and prop spread forPrompt.
Allows the parent to control the prompt while passing essential props seamlessly. No issue found.wren-ui/src/apollo/server/repositories/askingTaskRepository.ts (2)
23-25: Interface extension aligns with repository pattern.Defining a specialized method
findByQueryId(queryId: string)directly onIAskingTaskRepositoryis a clear and consistent approach. It improves discoverability and encapsulation of domain-specific queries.
58-70: Robust JSON stringify logic.Serializing
jsonbColumnsbefore storing is correct. If there's any chance of cyclical references or large structures, consider safe stringification with error handling or logging to prevent silent failures or truncated data.Do you want me to generate a script to check for cyclical JSON data in the
detailcolumn upstream in the codebase?wren-ui/src/apollo/server/services/askingTaskTracker.ts (2)
342-349: Parallel job handling done right – but confirm concurrency limit.Using
Promise.allSettledfor parallel job execution is a valid pattern. For a large number of tasks, you may want to limit concurrency to avoid saturating your service or external AI adaptor calls.Would you like a script to track the max concurrency over a given time period by analyzing logs or checking task creation frequency? This helps verify load constraints.
37-54: Interfaces and structure are clear.Defining the
TrackedTask,TrackedAskingResult, and handlingrerunFromCancelledlogic increateAskingTaskare well-structured and easy to follow.wren-ui/src/components/pages/home/prompt/Result.tsx (1)
80-87: Effective use of attachLoading.Attaching the loading state to the stop action is a nice pattern for user feedback, preventing multiple concurrent cancellations and offering a robust UX.
wren-ui/src/hooks/useAskPrompt.tsx (10)
2-2: No concerns with the new lodash imports.
7-7: ImportingDetailedThreadaligns with new cache update logic.
10-10: ImportingThreadResponseis consistent with the new rerun logic.
16-16: NewuseRerunAskingTaskMutationimport looks good.
21-23: Additional imports forTHREAD,ApolloClient, andnextTicklook fine.
160-160: Rerun mutation usage
Definingconst [rerunAskingTask] = useRerunAskingTaskMutation();is consistent with the new rerun flow. No issues here.
224-236: Well-structured effect
TheuseEffectblock that updates the cache whenisNeedPreparingis true is appropriate. Just confirm that a re-render won’t trigger re-entrant calls whencheckFetchAskingStreamTaskupdates the same data.
283-298: Initiating new tasks
onSubmitandonFetchingappear consistent with the creation & polling flow. Looks good overall.
71-73:Details
✅ Verification successful
Check all call sites
isNeedPreparingis straightforward, but ensure that no external calls rely on a previously removed or renamed function.
🏁 Script executed:
#!/bin/bash # Searching references to isNeedPreparing to ensure no mismatch in usage. rg "isNeedPreparing"Length of output: 181
Verification Complete - No Issues Found with
isNeedPreparingCall SitesAfter executing the verification script, all references to
isNeedPreparingare confined to its declaration and usage within the same file (wren-ui/src/hooks/useAskPrompt.tsx). There are no external call sites relying on previously removed or renamed functionality. Please proceed with confidence.
112-152:Details
❓ Verification inconclusive
Potential mismatch in forced status
handleUpdateRerunAskingTaskCacheforcibly changesUNDERSTANDINGtoSEARCHING. Confirm that this transition is always valid for every rerun scenario to avoid unexpected states.
🏁 Script executed:
#!/bin/bash #!/bin/bash # Searching for usage of handleUpdateRerunAskingTaskCache to confirm forced status transitions rg "handleUpdateRerunAskingTaskCache"Length of output: 190
Action: Validate Forced State Transition in Asking Task
The function
handleUpdateRerunAskingTaskCachecurrently overrides any task with a status ofUNDERSTANDINGby setting it toSEARCHING(and updating the type toTEXT_TO_SQL). Our search confirms that this function is only referenced withinwren-ui/src/hooks/useAskPrompt.tsx. Please verify that this forced transition is intentional and valid across all rerun scenarios. If the change is always correct, consider adding an inline comment explaining the rationale; otherwise, adjust the logic to accommodate cases where retaining theUNDERSTANDINGstate might be necessary.wren-ui/src/apollo/server/services/askingService.ts (16)
28-28: ImportingProjectis aligned with changes requiring project context.
63-63: ReplacingviewIdwithtrackedAskingResult
This indicates deeper integration with the new tracked result flow. No immediate concerns.
100-112: ExtendedcreateAskingTasksignature
The additional parameters (rerunFromCancelled,previousTaskId,threadResponseId) enhance flexibility. Ensure existing calls outside this file pass correct defaults to avoid accidental null references.
113-116:rerunAskingTaskmethod introduction
Method signature is clear and allows rerunning from athreadResponseId. Good addition.
127-131: Doc updates
Comments for the new optional parameters are helpful. Keep them up to date if logic evolves further.
137-158:cancelAskingTaskupdates
Now delegating toaskingTaskTracker.cancelAskingTask. Maintaining error telemetry is a good practice.
540-543: Optional rerun input
ExpandingcreateAskingTaskto handle rerun logic is consistent with the new states. Implementation is correct.
550-561: Gathering histories
Good approach to filter out the canceled response ID. This ensures the new rerun does not include canceled context.
563-564: Synchronized return shape
Returning{ id: response.queryId }matches theTaskinterface. Looks consistent.
603-603: Maintaining telemetry
Your consistent telemetry usage for canceling tasks aids in debugging. No suggestions here.
611-616: Confirming null returns
getAskingTaskreturningnullmust be handled. The try-catch is done at the resolver layer as per learnings. That’s consistent.
617-621:getAskingTaskById
Similarly, returningnullis consistent with the approach forgetAskingTask.
626-629: Process doc
The step-based docstring for creating a thread is updated. Very helpful for onboarding.
642-643: LinkingaskingTaskId
CombiningtrackedAskingResult?.taskIdwith the new bind inaskingTaskTrackeris a good design to keep things in sync.
646-653: Binding logic
Appropriately binds the newly created thread response with the underlying tracked task. Straightforward.
702-710: Same pattern for newly created responses
Ensures the response has a consistentaskingTaskIdif present. No concerns here.wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx (12)
1-2: NewuseEffectanddebounceimports
These expansions indicate auto-generation of answers. Looks fine.
10-13: ImportedRobotSVG,ANSWER_TAB_KEYS,canGenerateAnswer
These re-organizations support the new tab-based UI and "Save to Knowledge" flow.
20-22: RefactoredgetAnswerIsFinished
Exporting this fromTextBasedAnswerhelps keep the logic DRY.
24-25:Preparationcomponent
Adding the new preparation UI is a welcome improvement for better user feedback.
26-29: Expanded imports forAskingTaskStatus,ThreadResponse, etc.
These align with the updated task flow.
177-210:isAnswerPreparedlogic
Combining!!answerDetail?.queryIdandgetAnswerIsFinishedis good. Just ensure we handlenullor undefined states.
217-231: Tab activation
onTabClicktriggers generation for breakdowns or charts if missing. This is a neat incremental fetch approach. No concerns.
235-246: Conditional rendering
showAnswerTabsensures we only see the tabs when the task is finished or the answer is prepared. This helps keep the UI from being cluttered.
250-260: Answer Tab
The'Answer'tab fetchesTextBasedAnswerif not in breakdown-only mode. This guard is correct to avoid mismatched states.
283-285: Beta Chart Tag
Labeling the chart feature as Beta is helpful for setting user expectations.
287-290:ChartAnswerusage
Providing a separate tab for chart functionality organizes the UI well.
292-324: “Save to Knowledge" and “Save as View”
The synergy between the knowledge base and saving queries as views is nicely integrated. No security concerns found.wren-ui/src/apollo/client/graphql/__types__.ts (7)
31-37: Additions to AskingTask fields look well-structured
These new optional fields (invalidSql,queryId,rephrasedQuestion,retrievedTables,sqlGenerationReasoning,traceId) provide rich metadata about the task and help with debugging or analytics.
147-147: SwappingviewIdwithtaskIdin CreateThreadInput
This replacement aligns the thread creation flow with the new “asking task” concept. It seems coherent and does not appear to introduce backward-incompatible changes.
153-153: Mirroring taskId in CreateThreadResponseInput
Ensuring both thread and thread response structures supporttaskIdfosters consistency and a more unified way to track the origin of the data.
561-561: NewrerunAskingTaskfield
Introducing this mutation in theMutationtype is a welcome addition, especially if you plan to reuse the logic from a prior response or re-runnable state.
913-913: ChangedaskingTaskfield to optional
Allowing theaskingTaskto beMaybe<AskingTask>makes sense if certain queries do not retrieve or associate a task. The approach looks consistent with other optional fields.
1152-1152: OptionalaskingTaskin ThreadResponse
Same reasoning as above; making it optional avoids forcing a task to exist for every response. This retains flexibility.
1157-1157: Relaxedsqlto optional
Changingsqlfrom required to optional lets you handle flows that produce no SQL. This also prevents forced placeholders.wren-ui/src/apollo/client/graphql/home.generated.ts (14)
14-15: Expanded CommonAskingTaskFragment
Including fields likerephrasedQuestion,invalidSql, and more ensures the UI has complete details for rendering or logging.
16-16: CommonResponseFragment referencing askingTask
ReferencingaskingTaskdirectly in the response fragment makes the relationship more explicit. This appears to streamline UI consumption.
30-30: Optional retrieval of askingTask
In theAskingTaskQuery, the field is now optional. This avoids errors when tasks are missing or incomplete.
42-42: ThreadQuery updates
This change indicates the query has been refined to carry newly added or modified fields in the nested data.
49-49: ThreadResponseQuery referencing optional sql
Ensuringsqlcan be null avoids forcing placeholders and supports incomplete states or new flows.
65-67: RerunAskingTaskMutationVariables
Introduces theresponseIdparameter, forming the basis for more advanced re-execution flows.
70-70: RerunAskingTaskMutation
This complements the new server-side capability to rerun tasks, returning aTaskobject for tracking.
85-85: CreateThreadResponseMutation with optionalsql
Extending the mutation to handle optional SQL and anaskingTaskreference is consistent with the new design.
166-166: GenerateThreadResponseBreakdownMutation
Enhances debugging or interactive exploring of the breakdown stages. The newly optional fields are effectively integrated.
173-173: GenerateThreadResponseAnswerMutation
Similarly updated to reflect optional fields in the thread response, ensuring consistent usage ofAskingTask.
180-180: GenerateThreadResponseChartMutation
Adapting the mutation to handle new optional data ensures robust, chart-friendly flows without forcing extraneous data.
188-188: AdjustThreadResponseChartMutation
This extension ensures you can adjust chart outputs while preserving optional states in the mutation’s response.
237-262: CommonAskingTaskFragmentDoc
Defining a separate GraphQL fragment forAskingTaskis a clean approach to unify usage in queries and ensure type safety across the codebase.
345-348: Query AskingTask references fragment
Replacing explicit fields with the fragment spread simplifies maintenance and keeps the type definitions consistent.wren-ui/src/apollo/server/resolvers/askingResolver.ts (21)
19-19: New import of AskingDetailTaskInput
This suggests we’re expanding how tasks are represented and passed around, improving clarity for the “asking” workflow.
29-29: New import of TrackedAskingResult
Indicates more advanced or extended tracking of results, which helps in re-running or post-processing tasks.
49-49: AddedrephrasedQuestion
Captures an LLM or system rephrasing for better clarity. Good addition to track user-facing transformations.
51-51: NewsqlGenerationReasoning
Providing reasoning details about SQL generation can be extremely helpful for debug logs and user insights.
52-52: NewretrievedTables
Displays which tables the system used or recognized. Useful for auditing or explaining query context.
53-53: NewinvalidSql
Key for debugging or user feedback, especially if generated SQL is syntactically incorrect or fails.
54-54: NewtraceId
Trace or correlation IDs are best practice for distributed debugging. Good addition.
55-55: NewqueryId
Identifies the query in logs or analytics. In concert withtraceId, helps with root-cause analysis.
79-79: BoundrerunAskingTask
Ensures proper scoping ofthisfor the new method. Straightforward improvement.
110-110: BoundtransformAskingTask
Similarly ensures the helper function is accessible with the correct context.
197-200: Null check on missing askResult
This avoids potential runtime errors when the result is absent, returning a cleannullinstead. Good defensive coding.
223-223: CallingtransformAskingTask
Centralizing the transformation logic fosters maintainability and consistent output structure.
231-232: AddedtaskIdto createThread’s input
This lines up with the new ability to create threads from an existing task. The descriptive comment clarifies usage.
243-259: Conditional handling oftaskId
Smartly retrieves an existing askingTask if the user is referencing a prior flow, else defaults to standard flow. This is a neat design pattern.
262-262: Creating thread after constructing threadInput
The approach ensures that logic validating thetaskIdor direct input is fully resolved before proceeding. Good flow.
302-302: StoredaskingTaskId
Retaining the link to the original or newly created “asking task” ensures future references, reruns, or debugging remain consistent.
373-374: SametaskIdaddition for createThreadResponse
Mirrors the approach in createThread to unify logic. This enforces a consistent thread flow.
385-401: Conditional logic for response creation
Replicates the “check taskId then fallback” approach. This symmetrical pattern is both intuitive and easier to maintain.
421-437: AddedrerunAskingTaskresolver
Allows reusing the same prompt or logic, which can be invaluable in iterative or error-correction flows. This is a solid addition.
594-600: Async retrieval ofaskingTaskby ID
Retrieving the stored record ensures the UI can reflect the correct state or details for the response. Good encapsulation withtransformAskingTask.
629-666:transformAskingTaskhelper method
This is a neat approach to unify logic for building a consistentAskingTask. Your usage ofPromise.all()for candidate resolution is efficient and well-structured.
…asking & generating flow
…with finished directly
andreashimin
force-pushed
the
feat/reasoning-ui
branch
from
a553054 to
8289bf7
Compare
andreashimin
force-pushed
the
feat/reasoning-ui
branch
from
8289bf7 to
6be8a3c
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
wren-ui/src/components/pages/home/promptThread/ChartAnswer.tsx (1)
92-101: 🛠️ Refactor suggestionUse ErrorHandler system instead of console.error
Based on the retrieved learnings, error handling for dashboard operations should be implemented through the ErrorHandler system instead of handling errors inline with
console.error.const [previewData, previewDataResult] = usePreviewDataMutation({ - onError: (error) => console.error(error), + onError: (error) => ErrorHandler.handle(error), }); const [createDashboardItem] = useCreateDashboardItemMutation({ - onError: (error) => console.error(error), + onError: (error) => ErrorHandler.handle(error), onCompleted: () => { message.success('Successfully pinned chart to dashboard.'); }, });Additionally, you'll need to import the ErrorHandler:
+import { ErrorHandler } from '@/utils/errorHandler';
♻️ Duplicate comments (1)
wren-ui/src/components/pages/home/preparation/index.tsx (1)
102-105:⚠️ Potential issueHandle task failure while polling still continues
As noted in a previous review comment, there's an issue where the task request might fail while polling for instant recommended questions continues. Consider adding explicit error handling for this case.
🧹 Nitpick comments (7)
wren-ui/src/components/pages/home/prompt/Input.tsx (1)
33-35: Add proper type annotation for the event parameterThe
syncInputValuefunction has an untyped event parameter. You should specify the type for better type safety and code maintainability.- const syncInputValue = (event) => { + const syncInputValue = (event: React.ChangeEvent<HTMLTextAreaElement>) => { setInputValue(event.target.value); };wren-ui/src/components/pages/home/preparation/index.tsx (2)
50-54: Add proper type annotation for the event parameterThe
onCancelfunction has an untyped event parameter. You should specify the type for better type safety and code maintainability.- const onCancel = (e) => { + const onCancel = (e: React.MouseEvent) => { e.stopPropagation(); const stopAskingTask = attachLoading(onStopAskingTask, setStopLoading); stopAskingTask(askingTask.queryId); };
56-60: Add proper type annotation for the event parameterSimilar to the previous comment, the
onReRunfunction also has an untyped event parameter.- const onReRun = (e) => { + const onReRun = (e: React.MouseEvent) => { e.stopPropagation(); const reRunAskingTask = attachLoading(onReRunAskingTask, setReRunLoading); reRunAskingTask(data); };wren-ui/src/components/pages/home/prompt/index.tsx (2)
115-119: Dedicated function for text-to-sql answer creation.
Allows isolated logic for “intent SQL” flow. As above, consider catching async errors explicitly.
135-142: Submission flow triggers state transitions & result display.
The function handles concurrency of “already processing” checks. Consider wrapping the await call in a try/catch block for more robust error handling.wren-ui/src/hooks/useAskProcessState.tsx (1)
87-148:ProcessStateMachineclass for robust transitions.
Encapsulating transitions in a static-only class is effective, though the linter suggests using straight functions or referencing the class name instead ofthis. Consider adjusting to quell lint.Example fix for lines referencing
this:- return this.transitions[from]?.next.includes(to) + return ProcessStateMachine.transitions[from]?.next.includes(to)🧰 Tools
🪛 Biome (1.9.4)
[error] 87-148: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 130-130: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 137-137: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
wren-ui/src/components/pages/home/prompt/Result.tsx (1)
158-158: Use optional chaining syntax for cleaner code.The conditional check for
onIntentSQLAnswercan be simplified with optional chaining.- onIntentSQLAnswer && onIntentSQLAnswer(); + onIntentSQLAnswer?.();🧰 Tools
🪛 Biome (1.9.4)
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🛑 Comments failed to post (1)
wren-ui/src/components/pages/home/preparation/index.tsx (1)
102-125: 💡 Verification agent
🧩 Analysis chain
Add null/undefined checks for askingTask properties
When accessing
askingTask.retrievedTablesandaskingTask.sqlGenerationReasoning, there are no checks to ensure these properties exist. This could lead to runtime errors if they are undefined.- const retrievedTables = askingTask.retrievedTables || []; - const sqlGenerationReasoning = - askingTask.sqlGenerationReasoning || askingStreamTask || ''; + const retrievedTables = askingTask?.retrievedTables || []; + const sqlGenerationReasoning = + askingTask?.sqlGenerationReasoning || askingStreamTask || '';Handle the possibility of an undefined
askingTaskby implementing more defensive coding throughout the component:
🏁 Script executed:
#!/bin/bash # Check if there are other places in preparation/index.tsx where askingTask is accessed without null checks rg --context 2 "askingTask\." wren-ui/src/components/pages/home/preparation/index.tsxLength of output: 1072
Enforce Consistent Null-Checks for
askingTaskThe current implementation adds optional chaining only for
retrievedTablesandsqlGenerationReasoningat one location. However, the shell script output shows that there are other instances—such as in theonCancelfunction (withaskingTask.queryId) and multiple status checks—whereaskingTaskis used without ensuring it’s defined. It would be safer to either:
- Add a robust null/undefined check upfront: Return early or disable interactions if
askingTaskisn’t valid.- Apply optional chaining consistently: For example, use
askingTask?.queryIdandaskingTask?.statusin all instances, ensuring no runtime errors occur ifaskingTaskisnullorundefined.Suggested changes:
- Before any property access: Check if
askingTaskis falsy and handle appropriately (e.g., return early).- In property accesses: Update all instances to use optional chaining where direct null checks aren’t employed.
- const retrievedTables = askingTask.retrievedTables || []; - const sqlGenerationReasoning = - askingTask.sqlGenerationReasoning || askingStreamTask || ''; + const retrievedTables = askingTask?.retrievedTables || []; + const sqlGenerationReasoning = + askingTask?.sqlGenerationReasoning || askingStreamTask || '';Also, please review all other occurrences in the file (e.g., in
onCanceland status checks) to ensure they either run after a comprehensive null-check or use optional chaining.📝 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.const processState = useMemo( () => convertAskingTaskToProcessState(askingTask), [askingTask], ); const [isActive, setIsActive] = useState( processState !== PROCESS_STATE.FINISHED, ); // wrapping up after answer is prepared useEffect(() => { setIsActive(!isAnswerPrepared); }, [isAnswerPrepared]); if (askingTask === null) return null; const showView = view !== null; // General steps const showRetrieving = retrievingNextStates.includes(processState) && !view; const showOrganizing = organizingNextStates.includes(processState) && !view; const showGenerating = generatingNextStates.includes(processState) && !view; const isStopped = askingTask.status === AskingTaskStatus.STOPPED; const retrievedTables = askingTask?.retrievedTables || []; const sqlGenerationReasoning = askingTask?.sqlGenerationReasoning || askingStreamTask || '';
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
wren-ui/src/components/pages/home/prompt/Input.tsx (2)
33-35: Add proper type for the event parameterThe
syncInputValuefunction has an untyped event parameter, which could lead to type safety issues.- const syncInputValue = (event) => { + const syncInputValue = (event: React.ChangeEvent<HTMLTextAreaElement>) => { setInputValue(event.target.value); };
37-42: Consider adding error handling for the PromiseThe
handleAskfunction callsonAskwhich returns a Promise, but there's no error handling for potential failures.const handleAsk = () => { const trimmedValue = inputValue.trim(); if (!trimmedValue) return; const startAsking = attachLoading(onAsk, setInnerLoading); - startAsking(trimmedValue); + startAsking(trimmedValue).catch(error => { + console.error('Error during ask operation:', error); + // Consider adding user-facing error feedback here + }); };wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (2)
2-2: Refactor to avoid overshadowing the globalErrorobject.Consider renaming the imported
Errortype to something more descriptive (e.g.,GraphQLError) to reduce confusion and avoid clashes with the globalError.- import { Error } from '@/apollo/client/graphql/__types__'; + import { Error as GraphQLError } from '@/apollo/client/graphql/__types__';🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: 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-19: Clarify or rename this component to avoid confusion with a standard React error boundary.This component conditionally renders error content but does not implement the full error boundary lifecycle (e.g.,
componentDidCatch). Consider renaming it (e.g.,ErrorPanel) or refactoring it to use React’s native error boundary mechanism if the intention is to catch errors in the entire subtree.wren-ui/src/components/pages/home/preparation/step/Organizing.tsx (1)
21-27: Improve maintainability by memoizing scroll function.The
scrollBottomfunction is re-declared each render, which can trigger extra computations in the effect hooks. Consider wrapping it withuseCallbackor extracting it outside the component to enhance performance and readability.export default function Organizing(props: Props) { const $wrapper = useRef<HTMLDivElement>(null); const { stream, loading, error } = props; - const scrollBottom = () => { + const scrollBottom = useCallback(() => { if ($wrapper.current) { $wrapper.current.scrollTo({ top: $wrapper.current.scrollHeight, }); } - }; + }, []);wren-ui/src/components/pages/home/promptThread/store.tsx (1)
11-37: Add documentation to clarify the purpose of these store methods and fields.This interface includes multiple properties and callback methods but lacks docstrings. Adding brief explanations for each method’s purpose, expected parameters, and return values enhances code readability and maintainability.
wren-ui/src/components/pages/home/preparation/step/Retrieving.tsx (1)
8-12: Align prop name with UI terminology.The component uses "models" in the UI text but the prop is named "tables". For better code clarity and consistency, consider renaming the prop to match the terminology used in the UI.
interface Props { - tables: string[]; + models: string[]; loading?: boolean; error?: ErrorBoundaryProps['error']; }You would also need to update the prop usage throughout the component.
wren-ui/src/components/pages/home/preparation/index.tsx (2)
115-115: Consider replacing hardcoded step counts with dynamic values.The component uses hardcoded step counts (
'1 step'or'3 steps') which could become outdated if steps are added or removed from the workflow.- return <div className="gray-6">{showView ? '1 step' : '3 steps'}</div>; + const stepCount = showView ? 1 : (showRetrieving + showOrganizing + showGenerating); + return <div className="gray-6">{stepCount} {stepCount === 1 ? 'step' : 'steps'}</div>;
168-172: Consider refactoring error assignment logic for better readability.The current approach creates three separate error variables with complex conditions. This could be simplified to make the code more maintainable.
- const retrievingError = - !!error && retrievedTables.length === 0 ? error : null; - const organizingError = !!error && !sqlGenerationReasoning ? error : null; - const generatingError = - !!error && !(organizingError || retrievingError) ? error : null; + // Determine which stage has the error + let retrievingError = null; + let organizingError = null; + let generatingError = null; + + if (error) { + if (retrievedTables.length === 0) { + retrievingError = error; + } else if (!sqlGenerationReasoning) { + organizingError = error; + } else { + generatingError = error; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/index.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Generating.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Organizing.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Retrieving.tsx(1 hunks)wren-ui/src/components/pages/home/prompt/Input.tsx(1 hunks)wren-ui/src/components/pages/home/promptThread/store.tsx(1 hunks)wren-ui/src/hooks/useGlobalConfig.tsx(1 hunks)wren-ui/src/styles/components/button.less(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wren-ui/src/styles/components/button.less
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-ui/src/hooks/useGlobalConfig.tsx
🧰 Additional context used
🧬 Code Definitions (1)
wren-ui/src/components/pages/home/preparation/step/Retrieving.tsx (2)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (2) (2)
Props(4:7)ErrorBoundary(9:19)wren-ui/src/components/PageLoading.tsx (1) (1)
Spinner(39:41)
🪛 Biome (1.9.4)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx
[error] 2-2: 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)
🔇 Additional comments (10)
wren-ui/src/components/pages/home/prompt/Input.tsx (3)
37-42: Proper handling of empty inputs implementedGood job implementing input validation to prevent empty queries from being submitted. This avoids unnecessary API calls and improves user experience.
44-48: Proper event typing and handling implementedThe
inputEnterfunction properly types the event parameter and correctly handles the Enter key press, preventing default behavior appropriately.
1-77: Well-structured component with good state managementThe component is well-structured following React best practices. It properly manages state transitions, handles user input, and manages focus appropriately. The conditional disabling of UI elements based on processing state improves user experience.
wren-ui/src/components/pages/home/preparation/step/Generating.tsx (1)
14-40: Looks good, no concerns.wren-ui/src/components/pages/home/preparation/step/Retrieving.tsx (2)
38-38: LGTM! Table count is dynamic and terminology is consistent.The component correctly displays the number of models found dynamically using the array length, which addresses a previous concern about hardcoded values.
20-45: Well-structured component with good error handling.The component has a clear structure with appropriate error handling through ErrorBoundary, and conditionally renders content based on the loading state. The UX flow is logical - showing a spinner during loading and displaying the found models with tags when complete.
wren-ui/src/components/pages/home/preparation/index.tsx (4)
53-64: Good use of state machine for process flow management.The implementation of the state machine to manage different process states and transitions is well done. This approach makes the code more maintainable and easier to reason about, especially for complex workflows with multiple states.
147-150: Elegant error handling approach.The error object is properly constructed by combining the error data with SQL validation information, providing comprehensive error details to downstream components.
66-119: Well-separated PreparationStatus component with clear responsibilities.The PreparationStatus component is well-isolated and has clear responsibilities. It handles different UI states based on the task status, including processing, stopped, and finished states, which helps keep the main component cleaner.
134-146: Effective state management with useEffect and useMemo.Good use of React hooks to manage the component state. The useMemo hook properly memoizes the process state to prevent unnecessary recalculations, and the useEffect hook correctly handles state updates when the answer is prepared.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wren-ui/src/components/pages/home/preparation/index.tsx (2)
143-147: Consider adding null check for askingTaskThe error calculation depends on
askingTask?.errorandaskingTask?.invalidSql, but there's no early return ifaskingTaskis null here. Although line 149 adds this check, it would be safer to include it in the useMemo dependency array.const error = useMemo(() => { return askingTask?.error ? { ...askingTask.error, invalidSql: askingTask.invalidSql } : null; -}, [askingTask?.error, askingTask?.invalidSql]); +}, [askingTask]);
115-115: Consider using variables for step countsHardcoded values like "1 step" and "3 steps" could lead to maintenance issues if the number of steps changes. Consider using variables derived from the actual number of displayed steps.
- return <div className="gray-6">{showView ? '1 step' : '3 steps'}</div>; + const stepCount = showView ? 1 : 3; // Or calculate dynamically based on actual steps + return <div className="gray-6">{stepCount} {stepCount === 1 ? 'step' : 'steps'}</div>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/index.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Generating.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Organizing.tsx(1 hunks)wren-ui/src/components/pages/home/preparation/step/Retrieving.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-ui/src/components/pages/home/preparation/step/Organizing.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx
[error] 2-2: 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 (8)
wren-ui/src/components/pages/home/preparation/ErrorBoundary.tsx (2)
2-2: Rename imported Error type to avoid shadowing global ErrorThe current import shadows the global JavaScript Error object, which can lead to confusion. Rename this import to avoid potential issues.
-import { Error } from '@/apollo/client/graphql/__types__'; +import { Error as GraphQLError } from '@/apollo/client/graphql/__types__';Then update the Props interface:
export interface Props { children: React.ReactNode; - error?: Error & { invalidSql?: string }; + error?: GraphQLError & { invalidSql?: string }; }🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: 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)
15-21: Good implementation of error displayThe error boundary effectively separates the error presentation from the application logic, making error handling more maintainable.
wren-ui/src/components/pages/home/preparation/step/Generating.tsx (2)
4-8: Well-defined interface with optional flagsThe Props interface is well-designed with clear, optional boolean flags that control the component's display state.
19-33: Clear conditional rendering based on stateThe component effectively handles different states with appropriate messaging and visual indicators.
wren-ui/src/components/pages/home/preparation/step/Retrieving.tsx (2)
6-6: Consider renaming 'tables' prop to 'models'The prop name 'tables' is inconsistent with the UI text that refers to "models". For better maintainability and clarity, consider renaming the prop to match the terminology used in the UI.
interface Props { - tables: string[]; + models: string[]; loading?: boolean; }Then update references to this prop throughout the component.
34-34: Using dynamic count for models foundGood implementation of dynamic text that shows the actual number of models found.
wren-ui/src/components/pages/home/preparation/index.tsx (2)
73-83: Good pattern for loading state managementUsing the
attachLoadingutility with state setters for loading indicators is a clean pattern that keeps the component's behavior consistent.
208-248: Well-structured timeline with conditional stepsThe timeline implementation effectively shows the appropriate steps based on the current process state. The error boundary wrapping provides good error handling.
2 participants
Description
This PR refactors the user query flow for Text-to-SQL to clarify each step, ensuring a more structured and intuitive experience. It enhances the logical progression of the query process, improving clarity and usability while providing a more seamless interaction for users.
Screenshots
Remarks
Summary by CodeRabbit
New Features
Preparationcomponent for managing and displaying answer preparation status.ErrorBoundarycomponent for enhanced error visibility within the application.Improvements
HomeThreadcomponent.