-
Notifications
You must be signed in to change notification settings - Fork 219
feat: Create actual indexes and fix up state + error logic CLOUDP-317945 #6933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
let isShowSuggestionsButtonDisabled = !hasNewChanges; | ||
|
||
// Validate query upon typing | ||
try { | ||
parseFilter(inputQuery); | ||
|
||
if (!inputQuery.startsWith('{') || !inputQuery.endsWith('}')) { | ||
isShowSuggestionsButtonDisabled = true; | ||
} | ||
} catch (e) { | ||
isShowSuggestionsButtonDisabled = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: parsing the query on every re-render seems a big too excessive, let's pack this logic inside a useMemo
@@ -104,10 +106,13 @@ const QueryFlowSection = ({ | |||
}: SuggestedIndexFetchedProps) => Promise<void>; | |||
indexSuggestions: Record<string, number> | null; | |||
fetchingSuggestionsState: IndexSuggestionState; | |||
initialQuery: Document | null; | |||
initialQuery: string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type is incorrect (as the JSON.stringify that you still keep below indicates), you're still getting a document here, not a string, why the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right -- i was trying to fix the type but messed it up along the way. i fixed it!
: 'Error parsing query. Please follow query structure.', | ||
indexSuggestionsState: 'error', | ||
}); | ||
track('Error parsing query', { context: 'Create Index Modal' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add TODOs that point to a ticket for cleaning up these non real tracking events, I think you mentioned that there is one that should be part of the project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added TODOs and yes these will be cleaned up in wrap-up
…ed comments for cleaning
Description
Had to re-enable the button always to get this error to show up -- normally there's validation so user won't be able to click on the button in the first place with parsing errors


Also mocked this error by manipulating the # of rows. It is unlikely for the user to encounter this normally.


Drive-by fixes to open from nudge styling + lint

Checklist
Motivation and Context
Open Questions
Dependents
Types of changes