DGAI: Integrate HyDe utility with discourse node suggestions component - ENG-293 #163
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update overhauls the suggestion mechanism in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DiscourseContextOverlay
participant HyDEEngine
User->>DiscourseContextOverlay: Selects a page/discourse node
DiscourseContextOverlay->>DiscourseContextOverlay: Extract unique relation label-type triplets
DiscourseContextOverlay->>DiscourseContextOverlay: Filter candidate nodes
alt Valid candidates and relations exist
DiscourseContextOverlay->>HyDEEngine: runHydeSearch(candidates, relations)
HyDEEngine-->>DiscourseContextOverlay: Return similarity-ranked suggestions
DiscourseContextOverlay->>User: Show spinner, then display suggestions
else No valid candidates
DiscourseContextOverlay->>User: Show "No relevant relations found"
end
User->>DiscourseContextOverlay: Creates block from suggestion
DiscourseContextOverlay->>DiscourseContextOverlay: Remove used node from suggestions
Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/roam/src/components/DiscourseContextOverlay.tsx (2)
107-158: Functions well but could benefit from optional chainingThe
getUniqueLabelTypeTripletsfunction is well-structured and correctly extracts relationship details between nodes. However, the conditional checks on lines 125 and 135 can be simplified using optional chaining.- if (destinationNode && destinationNode.text && destinationNode.format) { + if (destinationNode?.text && destinationNode?.format) { targetNodeDetails = { text: destinationNode.text, format: destinationNode.format, }; }- if (sourceNode && sourceNode.text && sourceNode.format) { + if (sourceNode?.text && sourceNode?.format) { targetNodeDetails = { text: sourceNode.text, format: sourceNode.format, }; }🧰 Tools
🪛 Biome (1.9.4)
[error] 125-125: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
351-359: Consider extracting node transformation logicThis transformation logic is well-implemented but could be extracted into a separate helper function for better reusability, especially if similar transformations are needed elsewhere.
+ const transformToNodeWithEmbedding = (node: SuggestedNode): CandidateNodeWithEmbedding => ({ + uid: node.uid, + text: node.text, + type: node.type, + embedding: [] as number[], + }); + const runHydeSearch = async ({ currentSuggestions, currentNodeText, relationDetails, }: RunHydeSearchArgs): Promise<SuggestedNode[]> => { // ... existing validation ... try { - const candidateNodesForHyde = currentSuggestions.map((node) => { - const nodeWithEmbedding = { - uid: node.uid, - text: node.text, - type: node.type, - embedding: [] as number[], - }; - return nodeWithEmbedding as CandidateNodeWithEmbedding; - }); + const candidateNodesForHyde = currentSuggestions.map(transformToNodeWithEmbedding);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/DiscourseContextOverlay.tsx(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/roam/src/components/DiscourseContextOverlay.tsx (2)
apps/roam/src/utils/getDiscourseRelations.ts (1)
DiscourseRelation(12-19)apps/roam/src/utils/hyde.ts (4)
RelationDetails(27-31)SuggestedNode(23-25)CandidateNodeWithEmbedding(18-21)findSimilarNodesUsingHyde(321-375)
🪛 Biome (1.9.4)
apps/roam/src/components/DiscourseContextOverlay.tsx
[error] 125-125: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
apps/roam/src/components/DiscourseContextOverlay.tsx (6)
298-321: Good implementation of async search with proper state managementThe implementation of the HyDE search within the useEffect hook is well-structured with appropriate state management. The code correctly handles loading states, error handling, and updates the UI accordingly.
331-375: Well-structured HyDE search function with proper error handlingThe
runHydeSearchfunction is well-implemented with clear input validation, proper error handling, and returns the expected result. It correctly transforms the candidate nodes to include empty embeddings for the HyDE search.
337-349: Good input validation with early returnsThe input validation at the beginning of the function is excellent. Early returns for invalid inputs prevent unnecessary processing and potential errors.
424-443: Good UI feedback with loading state and empty state handlingThe UI implementation provides excellent feedback to users with a loading spinner during search and appropriate messaging when no results are found. This enhances the user experience by keeping them informed about the current state.
323-329: Excellent update to filtered list after node selectionThe
handleCreateBlockfunction correctly removes the selected node from the filtered list after adding it to the block, preventing duplicate suggestions and providing a better user experience.
216-264: Well-optimized context data computation with useMemoThe
suggestionContextDataimplementation efficiently usesuseMemoto avoid unnecessary recomputation of relationship triplets and valid types. The logic for determining valid types and unique relation triplets is well-structured.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Multiple clarifying questions/context needed which will likely trigger changes and another review.
|
Reminder: use the linear
|
|
Reminder: add a loom video to the PR to help explain the context. Especially for anything user-facing. |
I want to defer this to a later pr maybe the api routes one, the functionality would not work on this one, would require some merging to another branch resolving conflits then demoing it, other way is using dummy functions but then would have to remove them. Both cases require work that would only be for demo and would also be done again in a later branch anyway. |
@sid597 This is all context that should be added to the PR so that the reviewer (or anyone viewing it for that matter) would understand the intentions and choices made. |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/roam/src/components/DiscourseContextOverlay.tsx (1)
155-233: Well-structured logic for extracting relation details.The
suggestionContextDatauseMemo properly extracts and organizes relation details needed for the HyDE search. The code:
- Finds relations connecting to the current node
- Maps them to appropriate
RelationDetailsobjects- Filters valid relation types
The nested useMemo for
uniqueTripletscould be moved outside to avoid recreating functions on each render.Consider extracting the nested useMemo to its own variable:
- const uniqueTriplets = useMemo(() => { - const relatedNodeType = selfNode.type; - // ... rest of implementation - }, [relationsConnectingToSelf, selfNode.type, allNodes]); + const relatedNodeType = selfNode.type; + const uniqueTriplets = useMemo(() => { + // ... rest of implementation + }, [relationsConnectingToSelf, relatedNodeType, allNodes]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/components/DiscourseContextOverlay.tsx(6 hunks)apps/roam/src/utils/hyde.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/roam/src/components/DiscourseContextOverlay.tsx (1)
apps/roam/src/utils/hyde.ts (3)
SuggestedNode(22-24)RelationDetails(26-30)findSimilarNodesUsingHyde(319-373)
🔇 Additional comments (8)
apps/roam/src/utils/hyde.ts (1)
310-310: Good simplification of the code.The change to directly assign
fullNodetosuggestedNodeObjectwithout destructuring eliminates unnecessary code. This simplification is appropriate sinceembeddingproperty has been removed from theCandidateNodeWithEmbeddingtype, as confirmed in past reviews.apps/roam/src/components/DiscourseContextOverlay.tsx (7)
8-8: Nice addition of loading state and feedback.Adding the Spinner component import and introducing state variables for HyDE search status provides better user feedback during async operations.
Also applies to: 114-117
33-38: Good organization of imports from the hyde utility.Proper imports of the required types and functions from the hyde utility file support the integration with clear type definitions.
250-267: Effective type-narrowing with filter operators.Good use of TypeScript type predicates in the filter to ensure type safety, especially with the
filter((node): node is SuggestedNode => node !== null)pattern to narrow down the types.
269-292: Good async handling with proper loading states.The implementation properly manages loading states and error handling during the HyDE search:
- Sets loading state before search
- Clears results before fetching new ones
- Handles errors and resets state
- Uses finally block to ensure loading state is reset
This provides a robust user experience even when errors occur.
302-342: Well-structured HyDE search function with good error handling.The
runHydeSearchfunction is well-organized with:
- Clear parameter typing
- Input validation
- Try-catch blocks for error handling
- Appropriate logging of errors
This ensures that async operations are handled robustly.
391-410: Good UI feedback during loading and empty states.The UI properly shows:
- A spinner during the HyDE search
- The filtered nodes when available
- A "No relevant relations found" message when appropriate
This provides clear feedback to users about the state of the search operation.
294-300: Simple and effective node removal on add.The
handleCreateBlockfunction effectively updates the UI state by removing added nodes from the suggestion list, providing immediate feedback to the user.
There was a problem hiding this comment.
https://www.loom.com/share/af0f0551346a4b07b0d122eef86d3121
I get this error on trying to load just the overlay icon.
This means that the rest of the UI cannot be reviewed because it doesn't load.
We also can't confirm that the code is functional in any meaningful sense.
If this is intentional, please comment in the code (or in the GitHub PR UI) where/why this is happening, and how this works into the overall flow.
If a simple stub function can fix this, please add it. Eg:
PR Body / Loom Video:
This PR adds the UI and front end scaffolding for the upcoming backend HyDe search API's. There is a stub function that just returns the suggested nodes without sorting them.
// TODO: Remove this once the HyDE search is working
const tempFindSimilarNodesUsingHyde = async ({
candidateNodes,
currentNodeText,
relationDetails,
}: {
candidateNodes: SuggestedNode[];
currentNodeText: string;
relationDetails: RelationDetails[];
}): Promise<SuggestedNode[]> => {
return candidateNodes;
};
// TODO: Remove this once the HyDE search is working
const foundNodes: SuggestedNode[] = await tempFindSimilarNodesUsingHyde({
candidateNodes: candidateNodesForHyde,
currentNodeText: currentNodeText,
relationDetails: relationDetails,
});
// TODO: Uncomment this once the HyDE search is working
// const foundNodes: SuggestedNode[] = await findSimilarNodesUsingHyde({
// candidateNodes: candidateNodesForHyde,
// currentNodeText: currentNodeText,
// relationDetails: relationDetails,
// });
|
https://www.loom.com/share/aa5b0523f6ed4d8c829219e70a22ec33?sid=6f258300-ecdd-4492-a959-438d846e8c3a @mdroidian addressed all the review points, previously I was using cursor for all the work, I reviewed the code, parts of it seemed correct, things which did not I asked it to correct, it corrected the part i wanted it to fix but left the unused code from previous run. In my mind it was reviewed and only some new function was added which lead to these situation. For this run I actually reviewed using my brains instead of outsourcing to llm, I think its correct this time and covers the practices we want to follow. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/components/DiscourseContextOverlay.tsx(7 hunks)apps/roam/src/utils/hyde.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/roam/src/components/DiscourseContextOverlay.tsx (1)
apps/roam/src/utils/hyde.ts (2)
SuggestedNode(22-24)RelationDetails(26-30)
🔇 Additional comments (14)
apps/roam/src/utils/hyde.ts (1)
310-310: Direct use of fullNode improves code clarity.The change to directly use
fullNodeas the node property simplifies the code by avoiding unnecessary object restructuring. This aligns with previous feedback about redundant variable usage.apps/roam/src/components/DiscourseContextOverlay.tsx (13)
7-8: Added Spinner component for loading state indication.Good addition of the Spinner component from Blueprint for providing visual feedback during asynchronous operations.
31-35: Imported necessary types and functions from hyde utility.The imports have been updated to include the required functions and types for the HyDE-based search implementation.
111-114: Added state for HyDE search process.These new state variables appropriately track the loading state and results of the HyDE similarity search.
152-160: Simplified validRelations calculation.The logic to determine valid relations has been streamlined to filter relations where either the source or destination matches the self type.
162-198: Created relation details extraction for HyDE search.This memoized function extracts the necessary relation details (label, related node text, format) needed for the HyDE similarity search. The logic correctly handles both source and destination relationships.
200-217: Updated validTypes dependency on validRelations.The validTypes calculation now correctly depends on the validRelations computed earlier, maintaining proper dependency chains.
228-228: Reset HyDE filtered nodes when page selection changes.Good practice to clear previous results when the selection changes.
236-249: Enhanced node filtering logic.The node filtering process now:
- Checks for valid node types
- Uses proper type assertion with the SuggestedNode type
- Excludes nodes that already exist in the results
251-290: Implemented asynchronous HyDE search workflow.This implementation correctly:
- Sets loading state during search
- Prepares candidate nodes for the HyDE search
- Uses a temporary function with appropriate TODOs for future implementation
- Updates state with search results
- Includes proper error handling
- Cleans up loading state in finally block
The implementation is well-structured and handles edge cases appropriately.
293-303: Created temporary HyDE search function.The placeholder function maintains the expected interface while allowing development to continue before the final implementation is ready.
305-311: Updated handleCreateBlock to use SuggestedNode and maintain filtered list.The function now accepts a SuggestedNode parameter and updates the filtered list to remove nodes after they're added, preventing duplicate additions.
360-362: Added loading spinner during search.Good UI improvement to show a loading indicator during the asynchronous search operation.
364-379: Updated UI to display HyDE search results.The UI now conditionally renders:
- The filtered nodes with add buttons when available
- A helpful message when no relevant relations are found
- Nothing during the loading state
This provides a good user experience with appropriate feedback.
Summary by CodeRabbit
New Features
Bug Fixes