feat(wren-ui): Integrate API to implement instructions UI #1433
Conversation
fredalai
force-pushed
the
feature/instruction-ui
branch
from
450ebb4 to
087a5d4
Compare
fredalai
force-pushed
the
feature/instruction-ui
branch
from
087a5d4 to
34bd226
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces a comprehensive set of GraphQL types, mutations, and queries for managing instructions. It integrates these changes into front-end components such as dropdowns, modals, drawers, and a management page. The sidebar and learning guide flows have been updated to include instructions, while related enums, utilities, error handlers, and SVG assets have been added or modified. Additionally, SQL pair components have been refined with improved type definitions and label adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as ManageInstructions
participant G as GraphQL Client
participant S as API Server
U->>M: Open Instructions Management Page
M->>G: Execute LIST_INSTRUCTIONS query
G->>S: Fetch instructions data
S-->>G: Return instructions data
G-->>M: Provide instructions
U->>M: Click "Add Instruction" button
M->>M: Open InstructionModal
U->>M: Submit instruction details
M->>G: Execute createInstruction mutation
G->>S: Send mutation request
S-->>G: Return created instruction
G-->>M: Deliver mutation response
M->>U: Display updated instruction list
sequenceDiagram
participant U as User
participant L as LearningGuideManager
participant G as Guide System
U->>L: Access learning guide section
L->>L: Verify if KNOWLEDGE_GUIDE is completed
alt Guide not completed
L->>G: Trigger playKnowledgeGuide
G-->>U: Display guide popover with instruction steps
U->>G: Click "Got it"
G-->>L: Mark guide as completed
else Guide completed
L->>U: Proceed without guide
end
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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: 0
🧹 Nitpick comments (5)
wren-ui/src/components/pages/knowledge/GlobalLabel.tsx (1)
1-13: Consider making the component more flexibleWhile the current implementation is clean, consider enhancing reusability by accepting props for customization (e.g., custom text or additional class names).
-export default function GlobalLabel() { +interface GlobalLabelProps { + text?: string; + className?: string; +} + +export default function GlobalLabel({ text = 'Global', className = '' }: GlobalLabelProps) { return ( <> <GlobalOutlined className="mr-2" /> - <Text className="gray-9">Global</Text> + <Text className={`gray-9 ${className}`}>{text}</Text> </> ); }wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx (1)
169-169: Fix grammatical error in button text.There's a minor grammatical error in the button text.
- Add an Question + Add a Questionwren-ui/src/components/modals/InstructionModal.tsx (2)
169-169: Fix grammatical error in button text.There's a minor grammatical error in the button text.
- Add an Question + Add a Question
142-145: Consider adding length validation error message.While you've set a maxLength of 100 for matching questions, there isn't a corresponding error message in ERROR_TEXTS for when a user exceeds this limit. Consider adding a MAX_LENGTH error message similar to how it's done for SQL_PAIR.QUESTION.
wren-ui/src/pages/knowledge/instructions.tsx (1)
54-61: Consider enhancing error handling.The
onErrorfunction currently just logs errors to the console, which might not provide enough feedback to the user when failures occur.- onError: (error) => console.error(error), + onError: (error) => { + console.error(error); + message.error('There was an error processing your request.'); + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
wren-ui/src/apollo/client/graphql/__types__.ts(10 hunks)wren-ui/src/apollo/client/graphql/instructions.generated.ts(1 hunks)wren-ui/src/apollo/client/graphql/instructions.ts(1 hunks)wren-ui/src/components/diagram/CustomDropdown.tsx(2 hunks)wren-ui/src/components/modals/DeleteModal.tsx(1 hunks)wren-ui/src/components/modals/InstructionModal.tsx(1 hunks)wren-ui/src/components/pages/knowledge/GlobalLabel.tsx(1 hunks)wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx(1 hunks)wren-ui/src/components/pages/knowledge/utils.tsx(1 hunks)wren-ui/src/components/sidebar/Knowledge.tsx(1 hunks)wren-ui/src/components/sidebar/SidebarMenu.tsx(1 hunks)wren-ui/src/pages/knowledge/instructions.tsx(1 hunks)wren-ui/src/pages/knowledge/question-sql-pairs.tsx(2 hunks)wren-ui/src/utils/enum/index.ts(1 hunks)wren-ui/src/utils/enum/knowledge.ts(1 hunks)wren-ui/src/utils/enum/modeling.ts(1 hunks)wren-ui/src/utils/enum/path.ts(1 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)wren-ui/src/utils/errorHandler.tsx(2 hunks)wren-ui/src/utils/svgs/InstructionsSVG.tsx(1 hunks)wren-ui/src/utils/svgs/index.ts(1 hunks)wren-ui/src/utils/time.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
wren-ui/src/components/pages/knowledge/utils.tsx (1)
wren-ui/src/utils/enum/knowledge.ts (1) (1)
KNOWLEDGE(1-4)
wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx (1)
wren-ui/src/components/pages/knowledge/GlobalLabel.tsx (1) (1)
GlobalLabel(6-13)
wren-ui/src/components/sidebar/Knowledge.tsx (4)
wren-ui/src/utils/enum/knowledge.ts (1) (1)
KNOWLEDGE(1-4)wren-ui/src/utils/svgs/InstructionsSVG.tsx (1) (1)
InstructionsSVG(1-29)wren-ui/src/components/sidebar/SidebarMenu.tsx (1) (1)
SidebarMenu(70-83)wren-ui/src/components/pages/knowledge/utils.tsx (1) (1)
MENU_KEY_MAP(3-6)
wren-ui/src/components/modals/InstructionModal.tsx (1)
wren-ui/src/utils/error/dictionary.ts (1) (1)
ERROR_TEXTS(1-122)
wren-ui/src/apollo/client/graphql/instructions.generated.ts (1)
wren-ui/src/apollo/client/graphql/__types__.ts (4) (4)
Exact(4-4)CreateInstructionInput(114-118)InstructionWhereInput(495-497)UpdateInstructionInput(1272-1276)
🔇 Additional comments (45)
wren-ui/src/utils/enum/index.ts (1)
10-10: Export addition aligns with implementation of the Instructions featureThis export makes the knowledge enums available, which will be used for the new Instructions functionality.
wren-ui/src/utils/svgs/index.ts (1)
3-3: SVG component export aligns with Instructions feature UIThe new InstructionsSVG export provides visual assets needed for the Instructions UI.
wren-ui/src/utils/time.ts (1)
19-21: New time utility function for Instructions UI displayThe addition of
getCompactTimelooks good for formatting instruction timestamps. However, unlike other time functions in this file, it doesn't call.utc(). Is this intentional?If UTC conversion is needed for consistency with other time functions, consider:
export const getCompactTime = (time: string) => { - return dayJs(time).format('YYYY-MM-DD HH:mm'); + return dayJs(time).utc().format('YYYY-MM-DD HH:mm'); };wren-ui/src/utils/enum/path.ts (1)
12-12: Added path route for Instructions management pageThe new path follows the established naming convention and hierarchy, providing the route for the Instructions management feature described in the PR.
wren-ui/src/utils/enum/modeling.ts (1)
13-13: Addition of VIEW_INSTRUCTION matches the feature requirementsThis enum addition aligns with the PR's objective to implement an Instructions UI feature. It follows the existing naming pattern and will be used to enable viewing instruction details in dropdown menus.
wren-ui/src/utils/enum/knowledge.ts (1)
1-4: Well-structured constant for knowledge categoriesThe introduction of the KNOWLEDGE constant provides a centralized place for knowledge category identifiers, which is preferable to having string literals scattered throughout the codebase. This supports the new Instructions feature while maintaining the existing Question-SQL pairs functionality.
wren-ui/src/components/pages/knowledge/utils.tsx (1)
1-6: Clean implementation of menu mapping utilityThis utility creates a clear relationship between application paths and knowledge types, which will be useful for routing and menu highlighting in the knowledge section. Using computed property names with enum values is a good practice that maintains type safety and reduces the chance of typos.
wren-ui/src/components/pages/knowledge/GlobalLabel.tsx (1)
6-13: Reusable component for global instruction indicatorThe GlobalLabel component is well-structured and follows React best practices. It will provide consistent styling for indicating global instructions across the application.
wren-ui/src/components/modals/DeleteModal.tsx (1)
111-116: Implementation of DeleteInstructionModal looks good!The implementation follows the established pattern in the file for creating delete modals using the
makeDeleteModalfactory function. The configuration is consistent with other delete modals, and the content message matches the one used forDeleteQuestionSQLPairModal.wren-ui/src/utils/svgs/InstructionsSVG.tsx (1)
1-29: SVG implementation is well-structured and follows best practicesThe component is properly typed with TypeScript, uses appropriate defaults, and implements conditional fill styling based on the
fillCurrentColorprop. The SVG paths look correctly formatted and the component follows React functional component patterns.wren-ui/src/components/sidebar/SidebarMenu.tsx (1)
1-83: SidebarMenu component implementation looks goodThe component is well-structured with appropriate styling through styled-components. The StyledMenu properly customizes the Ant Design Menu with consistent styling that aligns with the application's design system. The functional component correctly passes through the required props.
wren-ui/src/utils/error/dictionary.ts (1)
111-121: Error messages for instructions are well-definedThe new INSTRUCTION section in ERROR_TEXTS follows the established pattern in the file and provides clear validation messages for instruction details, questions, and application mode.
wren-ui/src/components/diagram/CustomDropdown.tsx (2)
226-283: Implementation ofInstructionDropdownfollows best practices.The new
InstructionDropdowncomponent follows the established pattern of the existing dropdown components in the file, particularly theSQLPairDropdown. It correctly implements view, edit, and delete operations for instructions, with proper event handling to prevent propagation. The implementation is clean and consistent with the codebase.
16-16: LGTM!The import of
DeleteInstructionModalensures that the deletion confirmation functionality is available to the newInstructionDropdowncomponent.wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx (2)
1-63: Well-structured drawer component with clear information hierarchy.The drawer effectively displays instruction details, matching questions, and creation time in a clear, hierarchical manner. The component handles conditional rendering between global instructions and question-specific instructions properly.
34-41: Elegant handling of global instructions.The implementation correctly displays a
GlobalLabelcomponent and a descriptive text when instructions are global, making the UX clear for users.wren-ui/src/pages/knowledge/question-sql-pairs.tsx (2)
26-26: LGTM!Adding
Paragraphto the destructured Typography components for use in the question rendering.
90-92: Improved question display with built-in ellipsis functionality.The use of Ant Design's
Paragraphcomponent with the ellipsis property simplifies the code while maintaining the desired truncation functionality. This approach is better than using a custom styled component.wren-ui/src/components/modals/InstructionModal.tsx (3)
1-64: Well-structured modal with appropriate form validation.The modal implementation follows best practices with proper form validation, initialization, and submission handling. The form correctly handles both create and update modes, and includes appropriate error messages from the error dictionary.
65-114: Good implementation of instruction form fields.The instruction details field and the radio button group for specifying global vs. specific application are well implemented with clear labels and helpful extra information.
115-176: Dynamic question fields implemented correctly.The dynamic form fields for matching questions are correctly implemented using Ant Design's Form.List. The implementation includes proper validation and allows adding/removing questions with appropriate constraints.
wren-ui/src/pages/knowledge/instructions.tsx (4)
107-110: Good use of ellipsis for long instructions.I like that you're using the ellipsis property to truncate long instruction details while providing the full text in a tooltip when hovering.
117-141: Well-implemented conditional rendering for question display.Good job handling the different display cases: showing a global label for default instructions and implementing a clean "more questions" counter for instructions with many questions.
162-204: Well-structured UI with appropriate documentation link.The layout is clean with a clear title, descriptive help text, and a "Learn more" link to documentation. The table implementation with responsive settings is also well done.
213-222: Clean mutation handling in form submission.The submission handler elegantly handles both create and update cases in a single function, with appropriate variable structures for each mutation type.
wren-ui/src/utils/errorHandler.tsx (2)
324-349: Well-structured error handlers for instruction operations.The new error handlers follow the established pattern in the file, which is good for consistency. Each handler provides a clear, specific error message for its respective operation.
416-419: Good organization with clear comments.I appreciate the clear comment "// Instruction" that groups the related instruction error handlers, making the code more readable and maintainable.
wren-ui/src/components/sidebar/Knowledge.tsx (3)
10-20: Well-defined styled component for layout.The styled component provides good structure and styling for the sidebar layout, ensuring proper positioning and overflow handling.
27-48: Clean implementation of sidebar menu items.The menu items are well-structured with consistent formatting and appropriate icons. Using the enum values as keys is a good practice for maintaining consistency across the application.
50-56: Consistent integration with routing system.Good job using the mapping from pathname to menu keys, ensuring the correct menu item is highlighted based on the current route.
wren-ui/src/apollo/client/graphql/instructions.ts (3)
3-13: Well-structured GraphQL fragment.The Instruction fragment includes all necessary fields and follows best practices by being reused across multiple operations.
15-46: Clear and consistent GraphQL operation definitions.The query and mutation definitions follow best practices with proper parameter typing and fragment reuse. The mutations are well-structured with appropriate input types.
48-52: Appropriate deletion mutation.The DELETE_INSTRUCTION mutation is correctly implemented with just a return value indicating success, as no entity data needs to be returned after deletion.
wren-ui/src/apollo/client/graphql/__types__.ts (6)
114-118: Well-structured CreateInstructionInput typeThe type definition for creating instructions is clear and includes all necessary fields with proper types. The boolean
isDefaultwill help identify instructions that should apply to all queries.
484-493: Complete Instruction type with timestamp fieldsThe Instruction type is well-defined with all necessary fields including creation and update timestamps. This design will facilitate proper tracking of when instructions were created or modified, which is important for auditing and versioning.
495-497: Simple and effective InstructionWhereInputGood choice using a simple id-based query parameter for instruction operations. This approach is consistent with other entity types in the codebase and provides a clear, unique identifier for instructions.
550-550: Complete CRUD mutations for instructionsThe mutations for creating, deleting, and updating instructions are properly defined with appropriate return types. This provides a comprehensive API for managing instructions.
Also applies to: 559-559, 590-590
949-949: Query type for fetching instructionsThe instructions query returning an array of Maybe is appropriately defined, allowing for efficient retrieval of all instructions in a single query.
1272-1276: Flexible UpdateInstructionInput with optional fieldsGood design making all fields optional in the update input type. This allows for partial updates where only specific fields need to be modified without having to provide values for unchanged fields.
wren-ui/src/apollo/client/graphql/instructions.generated.ts (6)
6-6: Well-structured InstructionFragment typeThe fragment type includes all necessary fields from the Instruction type and will ensure consistency across all instruction-related operations.
35-45: Complete Instruction fragment definitionThe GraphQL fragment for Instructions is well-defined and includes all necessary fields. This approach promotes code reuse across queries and mutations.
46-52: Clean instructions query implementationThe Instructions query is concise and leverages the Instruction fragment for field selection. This approach ensures consistency and makes the query more maintainable.
80-86: Well-implemented mutation operationsAll three mutations (create, update, delete) are properly implemented with appropriate parameters and return values. The create and update mutations correctly reuse the Instruction fragment, while the delete mutation simply returns a boolean.
Also applies to: 113-119, 147-151
54-76: Well-documented query hook with examplesThe useInstructionsQuery hook is thoroughly documented with JSDoc comments that explain its purpose and provide usage examples. This will make it easier for developers to understand how to use the hook correctly.
89-109: Consistent implementation across mutation hooksAll mutation hooks follow the same pattern with thorough documentation, clear examples, and proper type definitions. This consistency will make the API more intuitive for developers to use.
Also applies to: 122-143, 154-174
fredalai
requested review from
andreashimin
and removed request for
andreashimin
There was a problem hiding this comment.
Here's some suggestions:
- The text on the Instruction Page needs to be revised to follow the First letter Capitalization rule. It looks many of form labels & table titles all letter capital
- This PR need to rebase if instruction API feat(wren-ui): instruction API #1392 updated, there's repeated code included.
onlyjackfrost
force-pushed
the
feat/instruction
branch
from
0d41aeb to
bac29f4
Compare
fredalai
force-pushed
the
feature/instruction-ui
branch
from
25e7b3d to
01b0fe8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wren-ui/src/components/modals/InstructionModal.tsx (1)
1-181: Looks excellent; consider additional error handling.Overall, the modal and form logic is well-structured, with proper field validation and usage of
destroyOnClose. You might consider adding an inline notification or error message in place of the currentconsole.errorto enhance the user experience for form submission failures.- .catch(console.error); + .catch((err) => { + console.error(err); + message.error('Instruction submission failed. Please check the console for details.'); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-ui/public/images/learning/instructions.pngis excluded by!**/*.png
📒 Files selected for processing (26)
wren-ui/src/apollo/client/graphql/__types__.ts(10 hunks)wren-ui/src/apollo/client/graphql/instructions.generated.ts(1 hunks)wren-ui/src/apollo/client/graphql/instructions.ts(1 hunks)wren-ui/src/components/diagram/CustomDropdown.tsx(2 hunks)wren-ui/src/components/learning/guide/stories.tsx(4 hunks)wren-ui/src/components/learning/guide/utils.ts(1 hunks)wren-ui/src/components/learning/index.tsx(1 hunks)wren-ui/src/components/modals/DeleteModal.tsx(1 hunks)wren-ui/src/components/modals/InstructionModal.tsx(1 hunks)wren-ui/src/components/modals/QuestionSQLPairModal.tsx(2 hunks)wren-ui/src/components/pages/knowledge/GlobalLabel.tsx(1 hunks)wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx(1 hunks)wren-ui/src/components/pages/knowledge/SQLPairDrawer.tsx(2 hunks)wren-ui/src/components/pages/knowledge/utils.tsx(1 hunks)wren-ui/src/components/sidebar/Knowledge.tsx(1 hunks)wren-ui/src/components/sidebar/SidebarMenu.tsx(1 hunks)wren-ui/src/pages/knowledge/instructions.tsx(1 hunks)wren-ui/src/pages/knowledge/question-sql-pairs.tsx(2 hunks)wren-ui/src/utils/enum/index.ts(1 hunks)wren-ui/src/utils/enum/knowledge.ts(1 hunks)wren-ui/src/utils/enum/modeling.ts(1 hunks)wren-ui/src/utils/enum/path.ts(1 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)wren-ui/src/utils/errorHandler.tsx(2 hunks)wren-ui/src/utils/svgs/InstructionsSVG.tsx(1 hunks)wren-ui/src/utils/svgs/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- wren-ui/src/utils/enum/index.ts
- wren-ui/src/utils/svgs/index.ts
- wren-ui/src/utils/enum/path.ts
- wren-ui/src/utils/svgs/InstructionsSVG.tsx
- wren-ui/src/utils/enum/knowledge.ts
- wren-ui/src/components/pages/knowledge/GlobalLabel.tsx
- wren-ui/src/utils/error/dictionary.ts
- wren-ui/src/components/modals/DeleteModal.tsx
- wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx
- wren-ui/src/utils/enum/modeling.ts
- wren-ui/src/components/pages/knowledge/utils.tsx
- wren-ui/src/pages/knowledge/question-sql-pairs.tsx
- wren-ui/src/utils/errorHandler.tsx
- wren-ui/src/apollo/client/graphql/instructions.ts
- wren-ui/src/components/sidebar/SidebarMenu.tsx
🧰 Additional context used
🧬 Code Definitions (4)
wren-ui/src/pages/knowledge/instructions.tsx (4)
wren-ui/src/components/pages/knowledge/GlobalLabel.tsx (1)
GlobalLabel(6-13)wren-ui/src/components/diagram/CustomDropdown.tsx (1)
InstructionDropdown(226-283)wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx (1)
InstructionDrawer(12-64)wren-ui/src/components/modals/InstructionModal.tsx (1)
InstructionModal(17-181)
wren-ui/src/components/learning/guide/stories.tsx (1)
wren-ui/src/components/learning/guide/utils.ts (1)
DriverPopoverDOM(5-5)
wren-ui/src/components/modals/InstructionModal.tsx (1)
wren-ui/src/utils/error/dictionary.ts (1)
ERROR_TEXTS(1-122)
wren-ui/src/apollo/client/graphql/instructions.generated.ts (1)
wren-ui/src/apollo/client/graphql/__types__.ts (4)
Exact(4-4)CreateInstructionInput(120-124)InstructionWhereInput(501-503)UpdateInstructionInput(1287-1291)
🔇 Additional comments (27)
wren-ui/src/components/learning/guide/utils.ts (1)
27-27: LGTM: Added Knowledge Guide to learning guidesThe addition of
KNOWLEDGE_GUIDEto theLEARNINGenum supports the new Instructions feature, enabling guidance for users on how to create and manage instructions.wren-ui/src/components/pages/knowledge/SQLPairDrawer.tsx (3)
5-5: LGTM: Improved type importsGood addition of the
SqlPairtype import for better type safety.
7-7: Improved type safety with specific SqlPair typeReplacing
anywith the specificSqlPairtype enhances type safety and makes the code more maintainable.
26-26: LGTM: Consistent label casingChanging "SQL Statement" to "SQL statement" provides consistent casing across the application.
wren-ui/src/components/learning/index.tsx (1)
309-309: LGTM: Updated guide referencesThe guide references have been properly updated to use
KNOWLEDGE_GUIDEinstead of the previousQUESTION_SQL_PAIRS_GUIDE, maintaining consistency with the enum changes.Also applies to: 313-314
wren-ui/src/components/modals/QuestionSQLPairModal.tsx (3)
15-15: LGTM: Improved type importsGood addition of the
SqlPairtype import for better type safety.
17-22: Enhanced type safety and flexibilityGood improvements:
- Replacing
anywith the specificSqlPairtype enhances type safety- Adding the optional
payloadproperty withisCreateModeadds flexibility to handle various scenarios like "Add a SQL pair from an existing answer"These changes make the component more robust and maintainable.
238-238: LGTM: Consistent label casingChanging "SQL Statement" to "SQL statement" provides consistent casing across the application.
wren-ui/src/components/diagram/CustomDropdown.tsx (2)
16-16: No issues with the new import.Importing
DeleteInstructionModalhere follows the existing pattern for otherDelete*Modalimports and looks consistent.
225-283: Ensure data presence checks.This new
InstructionDropdownreplicates the same structure as other dropdowns in this file, which is good for consistency. However, please ensure thatdatais always defined before usage in theonMoreClickcalls (especially in potential edge cases).Would you like to verify how
InstructionDropdownis being invoked throughout the codebase to confirm thatdatais never null or undefined?wren-ui/src/pages/knowledge/instructions.tsx (1)
1-232:❓ Verification inconclusive
Maintain consistency with pagination and real-time updates.
The table views and the
onMoreClickhandler are well-implemented. The integrated pagination at 10 items per page helps keep the UI manageable. For a seamless user experience, consider verifying if real-time state updates are needed after instruction creation/deletion or if the existing refetch strategies are sufficient.Run this script to confirm that instructions reappear accurately in the table immediately after creation or deletion:
🏁 Script executed:
#!/bin/bash # Verifies that newly created or deleted instructions are reflected after refetch. # 1) Check the query usage ast-grep --pattern $'useInstructionsQuery($_)' # 2) Confirm the presence of refetch or update logic rg -A 5 'refetchQueries|update(cache'Length of output: 426
Heads-up on UI refresh consistency:
The code leverages Apollo’s refetch strategy—usingrefetchQueries: ['Instructions']along withawaitRefetchQueries: true—to update the instructions list after create, update, or delete mutations. Pagination remains consistent at 10 items per page, and the table andonMoreClickhandler are well-implemented. However, because our automated verification of the refetch/update logic encountered a regex error, please manually verify that the instructions list reflects changes immediately after any mutation.
- Confirm that newly created instructions appear without delay.
- Ensure that deleted instructions are promptly removed from the table.
wren-ui/src/components/learning/guide/stories.tsx (3)
53-54: LGTM - Action mapping updated to use Knowledge GuideThe action mapping has been updated to replace
QUESTION_SQL_PAIRS_GUIDEwith the newKNOWLEDGE_GUIDE, correctly connecting to the newly addedplayKnowledgeGuidefunction.
367-446: Knowledge guide implementation looks goodThe
playKnowledgeGuidefunction properly replaces the previousplayQuestionSQLPairsGuide, adding support for instructions alongside Question-SQL pairs. The code correctly:
- Initializes the driver with appropriate config
- Sets up steps for guiding users through both Question-SQL pairs and Instructions
- Includes proper error handling
The new step for Instructions (lines 412-442) provides clear messaging about business rules and query logic.
🧰 Tools
🪛 Biome (1.9.4)
[error] 440-441: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
417-421: Verify the Instructions guide image appearanceThere was a previous comment about image height issues. Please verify that the new Instructions image displays correctly without any display problems.
wren-ui/src/components/sidebar/Knowledge.tsx (4)
1-9: LGTM - Updated imports for new functionalityImports have been properly updated to include styled-components, new icons, and the SidebarMenu component.
10-20: LGTM - Well-structured Layout componentThe new styled Layout component provides a clean, consistent structure for the sidebar with appropriate styling and positioning.
27-50: LGTM - Well-organized menu items structureThe menuItems array is well-structured, with consistent formatting for both Question-SQL Pairs and Instructions items. Each item properly includes:
- Data guide ID for testing/identification
- Link component with proper styling
- Appropriate icon
- Key for selection tracking
- Consistent className for styling
53-58: LGTM - Clean implementation of Layout and SidebarMenuThe render function now properly uses the new Layout component and SidebarMenu, providing a cleaner and more maintainable structure compared to the previous implementation.
wren-ui/src/apollo/client/graphql/__types__.ts (5)
120-124: LGTM - Well-defined CreateInstructionInput typeThe CreateInstructionInput type is properly structured with all necessary fields:
- instruction: String for storing the instruction text
- isDefault: Boolean flag for identifying default instructions
- questions: Array of strings for related questions
490-499: LGTM - Comprehensive Instruction type definitionThe Instruction type includes all necessary fields for the feature:
- Standard fields (id, projectId)
- Content fields (instruction, questions, isDefault)
- Metadata fields (createdAt, updatedAt)
501-503: LGTM - Well-structured input types for operationsThe input types (InstructionWhereInput, MutationCreateInstructionArgs, MutationDeleteInstructionArgs, MutationUpdateInstructionArgs) are properly defined with all necessary fields for their respective operations.
Also applies to: 640-642, 686-688, 832-835
556-556: LGTM - Complete set of CRUD operationsThe schema properly includes all necessary CRUD operations:
- Query: instructions (line 961)
- Mutations: createInstruction, deleteInstruction, updateInstruction (lines 556, 565, 597)
This provides a complete API for managing instructions.
Also applies to: 565-565, 597-597, 961-961
1287-1291: LGTM - Flexible UpdateInstructionInput typeThe UpdateInstructionInput type makes all fields optional (using InputMaybe), allowing for partial updates to instruction records which is a good practice.
wren-ui/src/apollo/client/graphql/instructions.generated.ts (4)
1-34: LGTM - Well-structured GraphQL type definitionsThe type definitions for fragments, queries, and mutations are properly structured with all necessary fields. The types correctly reflect the schema defined in types.ts.
35-52: LGTM - Complete GraphQL documentsThe GraphQL documents for the instruction fragment and query are well-structured:
- InstructionFragmentDoc includes all necessary fields
- InstructionsDocument properly reuses the fragment
54-79: LGTM - Well-documented query hooksThe useInstructionsQuery and useInstructionsLazyQuery hooks are properly implemented with:
- Detailed documentation and usage examples
- Proper options handling
- Appropriate return type definitions
80-112: LGTM - Complete set of mutation hooksThe mutation hooks (useCreateInstructionMutation, useUpdateInstructionMutation, useDeleteInstructionMutation) are properly implemented with:
- Detailed documentation and usage examples
- Proper options handling
- Appropriate type definitions
- Consistent implementation patterns across all mutations
Also applies to: 113-146, 147-177
fredalai
force-pushed
the
feature/instruction-ui
branch
from
01b0fe8 to
ca06b5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx (3)
16-22: Update thevisibleprop toopenfor Ant Design compatibility.The
visibleprop used in the Drawer component is deprecated in Ant Design v5. Consider using theopenprop instead to ensure compatibility with the latest version.<Drawer closable destroyOnClose onClose={onClose} title="View instruction" - visible={visible} + open={visible} width={760} >
10-10: Add documentation for the Props type.Consider adding explicit documentation or interface definition for the
Propstype to improve code maintainability and address the previous review comment about providing the defaultValue interface.-type Props = DrawerAction<Instruction>; +/** + * Props for the InstructionDrawer component + * @property {boolean} visible - Controls whether the drawer is visible + * @property {Instruction | undefined} defaultValue - The instruction data to display + * @property {() => void} onClose - Handler for when the drawer is closed + */ +type Props = DrawerAction<Instruction>;
28-28: Implement text truncation for instruction details.To maintain consistency with the table view in the instructions page, consider adding truncation for long instruction details using Ant Design's Typography.Paragraph component with ellipsis.
- <div>{defaultValue?.instruction || '-'}</div> + <Typography.Paragraph + ellipsis={{ rows: 5, expandable: true, symbol: 'more' }} + title={defaultValue?.instruction || '-'} + > + {defaultValue?.instruction || '-'} + </Typography.Paragraph>wren-ui/src/pages/knowledge/instructions.tsx (2)
59-66: Move utility function outside component.The
getBaseOptionsfunction is defined inside the component, causing it to be recreated on every render. Consider moving it outside the component to improve performance.+const getBaseOptions = (options) => { + return { + onError: (error) => console.error(error), + refetchQueries: ['Instructions'], + awaitRefetchQueries: true, + ...options, + }; +}; export default function ManageInstructions() { const instructionModal = useModalAction(); const instructionDrawer = useDrawerAction(); const { data, loading } = useInstructionsQuery({ fetchPolicy: 'cache-and-network', }); const instructions = data?.instructions || []; - const getBaseOptions = (options) => { - return { - onError: (error) => console.error(error), - refetchQueries: ['Instructions'], - awaitRefetchQueries: true, - ...options, - }; - };
176-178: Remove unnecessary empty className prop.The className prop with an empty string doesn't serve any purpose and can be removed.
<Button type="primary" - className="" onClick={() => instructionModal.openModal()} >wren-ui/src/components/learning/guide/stories.tsx (1)
427-437: Enhance instructions guidance with specific examples.The current description explains the concept of Instructions well, but it would be more helpful to include specific examples of when to use Instructions versus Question-SQL Pairs. This would help users understand the best use cases for each feature.
<> In addition to Question-SQL Pairs, you can create Instructions to define <b>business rules</b> and <b>query logic</b>. These rules guide Wren AI in applying consistent filters, constraints, and best practices to SQL queries. + <br /><br /> + <b>Example:</b> "Always filter out rows where the status is 'deleted'" or + "When querying sales data, join with the customer table to include customer names." </>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
wren-ui/src/apollo/server/services/instructionService.ts(1 hunks)wren-ui/src/components/learning/guide/stories.tsx(4 hunks)wren-ui/src/components/modals/InstructionModal.tsx(1 hunks)wren-ui/src/components/modals/QuestionSQLPairModal.tsx(2 hunks)wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx(1 hunks)wren-ui/src/components/pages/knowledge/SQLPairDrawer.tsx(2 hunks)wren-ui/src/pages/knowledge/instructions.tsx(1 hunks)wren-ui/src/pages/knowledge/question-sql-pairs.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- wren-ui/src/apollo/server/services/instructionService.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- wren-ui/src/components/modals/InstructionModal.tsx
- wren-ui/src/components/modals/QuestionSQLPairModal.tsx
- wren-ui/src/pages/knowledge/question-sql-pairs.tsx
- wren-ui/src/components/pages/knowledge/SQLPairDrawer.tsx
🧰 Additional context used
🧬 Code Definitions (2)
wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx (1)
wren-ui/src/components/pages/knowledge/GlobalLabel.tsx (1)
GlobalLabel(6-13)
wren-ui/src/components/learning/guide/stories.tsx (1)
wren-ui/src/components/learning/guide/utils.ts (1)
DriverPopoverDOM(5-5)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
wren-ui/src/components/learning/guide/stories.tsx (2)
416-422: Verify the image height is appropriate.In a previous review, there was a comment about the image height looking awkward. Ensure that the current height setting of 260px for the instruction guide image results in a visually balanced presentation.
Could you confirm if the new image height resolves the display issue mentioned in the previous review?
53-54: LGTM! Good replacement of the learning guide.The update to replace the
QUESTION_SQL_PAIRS_GUIDEwith the more comprehensiveKNOWLEDGE_GUIDEis a good enhancement that aligns with the PR objectives to implement the Instructions feature.
fredalai
force-pushed
the
feature/instruction-ui
branch
from
ca06b5a to
0e54107
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx (4)
10-10: Props type definition could be more specificThe
Propstype extendsDrawerAction<Instruction>, but it would be helpful to explicitly define or document what fields are in theInstructiontype for better clarity, particularly since a previous review requested information about thedefaultValueinterface.Consider adding more explicit type definitions or documentation:
/** * Props for InstructionDrawer * @defaultValue - Instruction object containing: * - instruction: string - The instruction text * - isDefault: boolean - Whether this is a global instruction * - questions: string[] - List of matching questions * - createdAt: string - Creation timestamp */ type Props = DrawerAction<Instruction>;
43-50: Add error handling for empty questions arrayThe component currently maps through
defaultValue?.questionswithout checking if it exists or is empty. Ifquestionsis undefined or an empty array, this could lead to rendering issues.- defaultValue?.questions.map((question, index) => ( + (defaultValue?.questions?.length > 0 ? + defaultValue.questions.map((question, index) => ( <div key={`${question}-${index}`} className="my-2"> <Tag className="bg-gray-1 border-gray-5"> <QuestionOutlined className="geekblue-6" /> <Text className="gray-9">{question}</Text> </Tag> </div> - )) + )) + : <Text className="gray-7">No matching questions</Text> + )
44-44: Improve key generation for mapped elementsUsing a combination of the question text and index as a key might not be optimal if questions can be duplicated or if they contain special characters that could cause issues when used in a key.
- <div key={`${question}-${index}`} className="my-2"> + <div key={`question-${index}`} className="my-2">
16-22: Consider adding accessibility attributes to the DrawerThe Drawer component could benefit from additional accessibility attributes to ensure it's usable by all users, including those using screen readers.
<Drawer closable destroyOnClose onClose={onClose} title="View instruction" visible={visible} width={760} + aria-labelledby="instruction-drawer-title" + role="dialog" > + <h2 id="instruction-drawer-title" className="visually-hidden">View Instruction Details</h2>wren-ui/src/components/modals/InstructionModal.tsx (5)
11-11: Consider adding a comment explaining the MAX_QUESTIONS limit.Adding a brief comment about why 100 was chosen as the maximum limit would help future developers understand this constraint.
-const MAX_QUESTIONS = 100; +// Limit maximum number of matching questions to prevent performance issues +const MAX_QUESTIONS = 100;
35-48: Consider improving error handling to provide user feedback.The form submission error is only logged to the console, which doesn't provide any feedback to the user when something goes wrong.
const onSubmitButton = () => { form .validateFields() .then(async (values) => { const data = { isDefault: values.isDefault, instruction: values.instruction, questions: values?.questions || [], }; await onSubmit({ data, id: defaultValue?.id }); onClose(); }) - .catch(console.error); + .catch((error) => { + console.error(error); + // Provide user feedback + Modal.error({ + title: 'Error', + content: 'Failed to submit the instruction. Please try again.', + }); + }); };
59-60: Useopenprop instead ofvisiblefor Ant Design Modal.The
visibleprop is deprecated in newer versions of Ant Design. Consider usingopeninstead for better future compatibility.- visible={visible} + open={visible} width={720}
88-89: Remove unnecessaryrequired={false}prop.The default value for the
requiredprop is alreadyfalse, so this prop is redundant.label="Apply instruction to" name="isDefault" - required={false} rules={[
149-158: Move inline styles to className for better maintainability.Inline styles should be avoided when possible. Consider moving them to a CSS class for better maintainability.
<Col flex="none" className="p-1"> <Button onClick={() => remove(name)} disabled={fields.length <= 1} icon={<DeleteOutlined />} size="small" - style={{ border: 'none' }} - className="bg-gray-1" + className="bg-gray-1 border-none" /> </Col>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
wren-ui/src/apollo/server/services/instructionService.ts(1 hunks)wren-ui/src/components/learning/guide/stories.tsx(4 hunks)wren-ui/src/components/modals/InstructionModal.tsx(1 hunks)wren-ui/src/components/modals/QuestionSQLPairModal.tsx(3 hunks)wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx(1 hunks)wren-ui/src/components/pages/knowledge/SQLPairDrawer.tsx(3 hunks)wren-ui/src/pages/knowledge/instructions.tsx(1 hunks)wren-ui/src/pages/knowledge/question-sql-pairs.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- wren-ui/src/apollo/server/services/instructionService.ts
- wren-ui/src/pages/knowledge/question-sql-pairs.tsx
- wren-ui/src/components/modals/QuestionSQLPairModal.tsx
- wren-ui/src/components/pages/knowledge/SQLPairDrawer.tsx
- wren-ui/src/pages/knowledge/instructions.tsx
🧰 Additional context used
🧬 Code Definitions (3)
wren-ui/src/components/modals/InstructionModal.tsx (1)
wren-ui/src/utils/error/dictionary.ts (1)
ERROR_TEXTS(1-122)
wren-ui/src/components/learning/guide/stories.tsx (1)
wren-ui/src/components/learning/guide/utils.ts (1)
DriverPopoverDOM(5-5)
wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx (1)
wren-ui/src/components/pages/knowledge/GlobalLabel.tsx (1)
GlobalLabel(6-13)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx (1)
1-64: LGTM: Well-structured component with clear sectionsThe InstructionDrawer component is well-organized with clear sections for instruction details, matching questions, and creation time. The conditional rendering based on whether it's a default instruction is handled appropriately, and the component provides fallbacks when data is missing.
wren-ui/src/components/modals/InstructionModal.tsx (4)
13-15: Specify what fields from Instruction are expected in defaultValue.The Props type extends ModalAction with Instruction but doesn't explicitly define which fields from Instruction are used as defaultValue. This was noted in a previous review comment.
-type Props = ModalAction<Instruction> & { +type InstructionFormValues = { + isDefault: boolean; + instruction: string; + questions?: string[]; +}; + +type Props = ModalAction<Pick<Instruction, 'id' | 'isDefault' | 'instruction' | 'questions'>> & { loading?: boolean; };
115-177: Good implementation of conditional form fields with proper validation.The conditional rendering of the matching questions field based on the
isDefaultvalue is well implemented, with appropriate validation rules and user guidance.
25-33: Form initialization is correctly implemented with proper dependencies.The useEffect hook correctly sets form values when the modal becomes visible and has the right dependencies in its dependency array.
1-10: The imports are appropriate and well organized.The component imports are logically grouped and include all necessary dependencies for the functionality.
wren-ui/src/components/learning/guide/stories.tsx (6)
53-54: Good update to the guide mapping in makeStoriesPlayerThe change from
QUESTION_SQL_PAIRS_GUIDEtoKNOWLEDGE_GUIDEcorrectly reflects the broader focus on knowledge building rather than just SQL pairs.
367-372: Function signature looks goodThe new
playKnowledgeGuidefunction maintains consistent parameter structure with other guide functions in the file.
384-409: Well-structured first step in the knowledge guideThe first step properly explains the Question-SQL Pairs concept with appropriate visuals and descriptive text. The popover styling with
resetPopoverStyleensures consistent UI.
411-437: New instructions feature is well-documentedThe second step clearly explains the Instructions feature, its purpose for defining business rules, and how it enhances Wren AI's behavior. The visual aid helps users understand the concept.
437-437: Good UX update with the button text changeChanging from "Get Started" to "Got it" better reflects the informational nature of this guide rather than suggesting the start of a new workflow.
416-420:❓ Verification inconclusive
Confirm the image dimensions are appropriate
Ensure the instructions.png image displays properly at the specified dimensions. There was a previous issue with image height in this file that needed fixing.
🏁 Script executed:
#!/bin/bash # Check if the image exists and get its dimensions if [ -f "public/images/learning/instructions.png" ]; then echo "Image exists. Checking dimensions..." file public/images/learning/instructions.png | grep -o "[0-9]* x [0-9]*" else echo "Image not found at public/images/learning/instructions.png" # Try to find it elsewhere fd -t f "instructions.png" --exec file {} \; | grep -o "[0-9]* x [0-9]*" fiLength of output: 291
Action Required: Confirm the instructions.png dimensions and its file location
It appears the automated verification didn’t locate the file at the expected path. The image might actually reside under the project’s “wren-ui/public/images/learning” directory rather than directly under “public/images/learning”. Please manually verify that:
- The image file is placed in the correct folder (likely
wren-ui/public/images/learning/instructions.png), and- Its dimensions (height especially) meet the requirements that resolved the previous height issue.
Once confirmed, ensure that the image renders properly with the expected dimensions.
fredalai
force-pushed
the
feature/instruction-ui
branch
from
0e54107 to
e9056e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
wren-ui/src/pages/knowledge/instructions.tsx (7)
61-61: Improve error handling for GraphQL mutations.The current error handling simply logs to console, but doesn't provide user feedback when operations fail.
- onError: (error) => console.error(error), + onError: (error) => { + console.error(error); + message.error('Operation failed. Please try again.'); + },
94-95: Add proper TypeScript type definitions for the payload parameter.The
payloadparameter lacks proper type definition, which could lead to type-related bugs.- const onMoreClick = async (payload) => { + const onMoreClick = async (payload: { type: MORE_ACTION; data: Instruction }) => {
161-162: Add aria-label to MoreButton for improved accessibility.The MoreButton component should have an aria-label to help screen reader users understand its purpose.
- <MoreButton className="gray-8" /> + <MoreButton className="gray-8" aria-label="More options" />
127-145: Consider handling empty questions array gracefully.The current implementation doesn't handle the case where
questionsmight be empty even thoughisDefaultis false.return ( <StyledQuestionsBlock> + {questions.length === 0 ? ( + <div className="text-sm gray-7 pl-1">No matching questions</div> + ) : ( {displayQuestions.map((question) => ( <div key={question} className="mb-1"> <StyledTag className="bg-gray-1 border-gray-5 text-truncate"> <QuestionOutlined className="geekblue-6" /> <Text className="gray-9" title={question}> {question} </Text> </StyledTag> </div> ))} {moreCount > 0 && ( <div className="text-sm gray-7 pl-1"> +{moreCount} more question{moreCount > 1 ? 's' : ''} </div> )} + )} </StyledQuestionsBlock> );
50-230: Consider breaking down the component into smaller, more focused components.The
ManageInstructionscomponent is quite large and handles multiple responsibilities: data fetching, UI rendering, and action handling. Breaking it down into smaller components would improve readability and maintainability.You could extract:
- A separate InstructionsTable component
- A separate ActionHandlers component or custom hook for the mutations and event handlers
This would make the main component more concise and focused on composition rather than implementation details.
167-228: Add error state handling for the table.The component should handle potential error states from the GraphQL query.
- const { data, loading } = useInstructionsQuery({ + const { data, loading, error } = useInstructionsQuery({ fetchPolicy: 'cache-and-network', }); // Then in the return statement: return ( <SiderLayout loading={false}> <div className="px-6 py-4"> + {error && ( + <div className="mb-3"> + <Alert + message="Error loading instructions" + description="Failed to load instructions. Please try refreshing the page." + type="error" + showIcon + /> + </div> + )} {/* Rest of the component */}Don't forget to add the Alert import:
import { Button, Tag, Table, TableColumnsType, Typography, message, + Alert, } from 'antd';
35-35: Consider edge cases in StyledQuestionsBlock styling.The negative margin styling could potentially cause layout issues with certain content or in different viewport sizes.
const StyledQuestionsBlock = styled.div` - margin: -2px -4px; + margin: -2px -4px; + display: flex; + flex-wrap: wrap; `;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
wren-ui/src/apollo/server/services/instructionService.ts(1 hunks)wren-ui/src/components/learning/guide/stories.tsx(4 hunks)wren-ui/src/components/modals/InstructionModal.tsx(1 hunks)wren-ui/src/components/modals/QuestionSQLPairModal.tsx(3 hunks)wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx(1 hunks)wren-ui/src/components/pages/knowledge/SQLPairDrawer.tsx(3 hunks)wren-ui/src/components/settings/utils.tsx(1 hunks)wren-ui/src/pages/knowledge/instructions.tsx(1 hunks)wren-ui/src/pages/knowledge/question-sql-pairs.tsx(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- wren-ui/src/components/settings/utils.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- wren-ui/src/apollo/server/services/instructionService.ts
- wren-ui/src/components/pages/knowledge/SQLPairDrawer.tsx
- wren-ui/src/components/modals/InstructionModal.tsx
- wren-ui/src/components/modals/QuestionSQLPairModal.tsx
- wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx
- wren-ui/src/pages/knowledge/question-sql-pairs.tsx
🧰 Additional context used
🧬 Code Definitions (2)
wren-ui/src/pages/knowledge/instructions.tsx (4)
wren-ui/src/components/pages/knowledge/GlobalLabel.tsx (1)
GlobalLabel(6-13)wren-ui/src/components/diagram/CustomDropdown.tsx (1)
InstructionDropdown(226-283)wren-ui/src/components/pages/knowledge/InstructionDrawer.tsx (1)
InstructionDrawer(12-64)wren-ui/src/components/modals/InstructionModal.tsx (1)
InstructionModal(17-181)
wren-ui/src/components/learning/guide/stories.tsx (1)
wren-ui/src/components/learning/guide/utils.ts (1)
DriverPopoverDOM(5-5)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
wren-ui/src/components/learning/guide/stories.tsx (6)
53-54: LGTM: Action mapping update aligned with new guide implementation.The update from
QUESTION_SQL_PAIRS_GUIDEtoKNOWLEDGE_GUIDEcorrectly maps to the new implementation of the guide function, maintaining consistency with the broader knowledge base approach.
367-381: LGTM: Well-structured guide function implementation.The function signature and initial setup follow the established pattern seen in other guide functions. The implementation handles edge cases properly with null checks and ensures proper cleanup of any active driver instances.
382-410: LGTM: First step implementation for Question-SQL Pairs.The first step of the guide provides clear information about Question-SQL Pairs with appropriate visuals and formatting. The popover sizing is consistent with other guide implementations.
411-437: LGTM: Instructions feature guide implementation.The new step for introducing the Instructions feature is well-structured with appropriate visuals and content that explains the purpose of Instructions for defining business rules and query logic.
437-437: Better button text for user interaction flow.Changing from "Get Started" to "Got it" better reflects the acknowledgment action rather than starting a new process, improving the user experience flow.
438-446: LGTM: Proper cleanup and event handling.The guide correctly handles cleanup by destroying the driver instance and triggering appropriate callbacks on completion, following the pattern established in other guide functions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 440-441: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
wren-ui/src/pages/knowledge/instructions.tsx (1)
94-105: Add confirmation dialog for delete operations.The delete functionality doesn't have a confirmation dialog, which could lead to accidental deletion of important instructions.
Consider implementing the Ant Design Modal.confirm approach as suggested in the previous review:
import { Button, Tag, Table, TableColumnsType, Typography, message, + Modal, } from 'antd'; const onMoreClick = async (payload) => { const { type, data } = payload; if (type === MORE_ACTION.DELETE) { + Modal.confirm({ + title: 'Delete Instruction', + content: 'Are you sure you want to delete this instruction?', + okText: 'Yes', + okType: 'danger', + cancelText: 'No', + onOk: async () => { await deleteInstructionMutation({ variables: { where: { id: data.id } }, }); + }, + }); } else if (type === MORE_ACTION.EDIT) { instructionModal.openModal(data); } else if (type === MORE_ACTION.VIEW_INSTRUCTION) { instructionDrawer.openDrawer(data); } };
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wren-ui/src/components/learning/guide/stories.tsx (2)
395-395: Title case inconsistency.The title "Build your knowledge base" uses lowercase "your" while other titles in the file use proper title case. For consistency, consider capitalizing this to "Build Your Knowledge Base" to match other titles in the guide.
- Build your knowledge base + Build Your Knowledge Base
423-424: Title case inconsistency in instructions title.Similar to the previous comment, for consistency with other titles in the file, consider using title case:
- Build your knowledge base with instructions + Build Your Knowledge Base with Instructions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-ui/src/components/learning/guide/stories.tsx(6 hunks)wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx(1 hunks)wren-ui/src/pages/knowledge/question-sql-pairs.tsx(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-ui/src/pages/knowledge/question-sql-pairs.tsx
🧰 Additional context used
🧬 Code Definitions (1)
wren-ui/src/components/learning/guide/stories.tsx (1)
wren-ui/src/components/learning/guide/utils.ts (1)
DriverPopoverDOM(5-5)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
wren-ui/src/components/learning/guide/stories.tsx (6)
53-54: Good update to the guide function reference.You've correctly updated the mapping to use the new
playKnowledgeGuidefunction instead of the previous SQL pairs guide, which aligns with the new instructions feature being introduced.
367-446: Well-implemented knowledge guide with comprehensive instructions support.The new
playKnowledgeGuidefunction properly replaces the previous SQL pairs guide, with the following improvements:
- Includes a new step for the instructions feature
- Updates the messaging to focus on building a knowledge base with both question-SQL pairs and instructions
- Uses appropriate imagery to illustrate the feature
The implementation looks good and aligns with the PR objective of introducing the Instructions feature.
🧰 Tools
🪛 Biome (1.9.4)
[error] 440-441: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
417-422: Verify the image height for instructions guide.The image element has a fixed
minHeightof 260px. Based on past review comments, there was an issue with image height in another part of the guide that required fixing.Please confirm that the image dimensions at
/images/learning/instructions.pngproperly fit within the specified 260px container height to avoid any display issues.
437-437: Good update to the button text.Changing the button text from "Get Started" to "Got it" better reflects the purpose of the button in this context, which is to acknowledge understanding rather than starting a process.
480-480: Title case consistency update.Good change to lowercase "to" in "Save to knowledge" which is more grammatically correct for prepositions in titles.
486-486: Consistent formatting for "question-SQL pairs".Good update to maintain consistent formatting of "question-SQL pairs" throughout the application, which improves the user experience.
fredalai
force-pushed
the
feature/instruction-ui
branch
from
ec6dadf to
1716702
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wren-ui/src/components/sidebar/Knowledge.tsx (3)
10-19: Consider extracting styled components to a separate file.The
Layoutstyled component looks well-defined with appropriate styling. However, as your component grows, you might want to consider moving styled components to a separate file to maintain better separation of concerns and keep the main component file focused on logic and structure.
21-21: Consider using styled-components for consistency.For consistency with the rest of the component's styling approach, consider converting the inline
linkStyleobject to a styled component:-const linkStyle = { color: 'inherit', transition: 'none' }; +const StyledLink = styled(Link)` + color: inherit; + transition: none; +`;Then use
StyledLinkin your menu items instead of wrappingLinkwith an inline style.
26-49: Well-structured menu items array with clear organization.The menu items are well-defined with appropriate data attributes for guide IDs, clear labels, icons, and keys. The addition of the Instructions menu item with its respective icon and path is implemented correctly.
Consider defining this array outside the component function to prevent it from being recreated on each render, which would be a minor optimization:
+const menuItems = [ + { + 'data-guideid': 'question-sql-pairs', + label: (props) => ( + <Link style={linkStyle} href={Path.KnowledgeQuestionSQLPairs}> + Question-SQL pairs + </Link> + ), + icon: <FunctionOutlined />, + key: KNOWLEDGE.QUESTION_SQL_PAIRS, + className: 'pl-4', + }, + // ...rest of items +]; export default function Knowledge() { const router = useRouter(); - const menuItems = [...];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wren-ui/src/components/learning/guide/stories.tsx(6 hunks)wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx(1 hunks)wren-ui/src/components/sidebar/Knowledge.tsx(1 hunks)wren-ui/src/pages/knowledge/question-sql-pairs.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wren-ui/src/components/pages/home/promptThread/AnswerResult.tsx
- wren-ui/src/pages/knowledge/question-sql-pairs.tsx
🧰 Additional context used
🧬 Code Definitions (1)
wren-ui/src/components/learning/guide/stories.tsx (1)
wren-ui/src/components/learning/guide/utils.ts (1)
DriverPopoverDOM(5-5)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
wren-ui/src/components/sidebar/Knowledge.tsx (2)
3-8: Good organization of imports.The imports are well organized and properly structured, including all necessary components for the sidebar menu implementation. The addition of the
InstructionsSVGandSidebarMenuimports aligns well with the new feature requirements.
52-57: Clean implementation of the SidebarMenu component.The implementation of the SidebarMenu component is clean and straightforward. The use of the
MENU_KEY_MAPto determine selected keys based on the current pathname is a good approach for tracking the active menu item.wren-ui/src/components/learning/guide/stories.tsx (8)
53-54: Function mapping updated to use the new guide function.The code now correctly maps
LEARNING.KNOWLEDGE_GUIDEto the newplayKnowledgeGuidefunction, replacing the previous SQL pairs guide. This change supports the new combined knowledge guide approach.
367-379: New guide function replaces SQL pairs guide with expanded functionality.The introduction of
playKnowledgeGuidefunction replaces the previousplayQuestionSQLPairsGuidefunction, expanding the scope to include both SQL pairs and instructions features. The function signature and initialization remain consistent with other guide functions.
384-396: First step updated with consistent terminology.The first step of the guide retains focus on Question-SQL pairs but updates the title to "Build knowledge base: Question-SQL pairs" for consistency with the new instructions step. The image path still points to the save-to-knowledge GIF.
399-404: Description text updated with proper emphasis.The description now properly emphasizes "Question-SQL pairs" with bold styling, improving readability and highlighting key concepts for users. The overall messaging about refining Wren AI's SQL generation remains clear.
411-437: New step added for Instructions feature.This second step introduces the new Instructions feature to users, explaining how it complements Question-SQL pairs by defining business rules and query logic. The step includes an image and clear description of the feature's benefits.
The image has a
minHeightof 260px which appears appropriate, addressing a previous height issue mentioned in past reviews.
437-437: Button text updated for better user experience.Changed the button text from "Get Started" to "Got it", which better reflects the informational nature of this guide step rather than prompting the user to begin a new action.
480-480: Consistent capitalization in guide terminology.Updated "Save to Knowledge" to "Save to knowledge" for consistency with other headings in the guides.
486-486: Added emphasis to key terminology.Applied bold styling to "Question-SQL pair" in the description, consistent with how this term is emphasized in other parts of the guide, improving visual hierarchy and readability.
Description
The Instructions feature allows users to define business rules, which will help Wren AI understand the data model and business rules, improving query accuracy and reducing the need for manual refinements.
Users can create global instructions that apply to all queries or match them to specific user questions based on similarity.
Tasks
UI screenshots
knowledge guide
Empty data
Add Modal
Add instruction & view
Update instruction & view
Delete instruction
Error State
Manage instructions UI page (with sample data)
Update for consistent casing
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor