feat: Fix the state management for the two flows (disabled state, displaying covered queries) CLOUDP-325467 #7036
Conversation
packages/compass-indexes/src/components/create-index-form/query-flow-section.tsx
Outdated
Show resolved
Hide resolved
packages/compass-indexes/src/components/create-index-form/index-flow-section.tsx
Outdated
Show resolved
Hide resolved
packages/compass-indexes/src/components/create-index-form/index-flow-section.tsx
Outdated
Show resolved
Hide resolved
packages/compass-indexes/src/components/create-index-form/query-flow-section.tsx
Outdated
Show resolved
Hide resolved
… query + initialQuery to redux
| }, []); | ||
| const handleQueryInputChange = useCallback( | ||
| (text: string) => { | ||
| onQueryUpdated({ query: text, hasQueryChanges: true }); |
There was a problem hiding this comment.
I mentioned a few times, with redux you really want your reducer and action types to be the driver of the logic, sort of (sorry, it's a bit vague). So the actual driver for this value changing is the fact of the action itself, query changed by the user so hasQueryChanges is now true, right? And so reducer can set it to true when it's reacting to the query updated action, it doesn't need to be passed as an argument. You can think about this also from this perspective: does it ever make sense to dispatch this action and have hasQueryChanges be set to false? As far as I can see it should never be the case, so shouldn't be something that action calls is driving
| // get it from the current query or initial query from insights nudge | ||
| query: state.query || parsedInitialQuery, |
There was a problem hiding this comment.
Not sure, but shouldn't parsedInitialQuery take precedence here? Like if something triggered opening a modal with a new query, it should replace whatever we had in the state before?
There was a problem hiding this comment.
i believe state.query should take priority because user can end up changing the query. parsedInitialQuery is only relevant when opening from the nudge so in that case state.query would be '' anyway
| collectionName: string; | ||
| inputQuery: string; | ||
| }): IndexesThunkAction< | ||
| query, |
There was a problem hiding this comment.
Not that important, but even this value you have in the state, so it can be removed from the arguments here if you want to make the interface even more enclosed
There was a problem hiding this comment.
i'll keep it because i dont think that the one in state is the most updated one
There was a problem hiding this comment.
It is always the most updated just by the nature of how redux works (and if it's not we're in trouble) 🙂 In case where you dispatch it inside the open thunk action, the previous action dispatch sets it in the store. In the UI where user dispatches it by clicking a button, it's always synced to store with queryUpdated action. Just FYI to understand the flow better
There was a problem hiding this comment.
oh wait yeah you're right i got it confused with something el se. but in this case:
query: JSON.stringify(initialQuery, null, 2) || '',
we are passing in the initialQuery instead of the query so maybe good to have it for distinction?
There was a problem hiding this comment.
That's kinda fair! You can look at it from different perspectives, I'd say from my perspective that the action is something that always fetches suggestions for the current query in the input, this also matches your open action handling logic in the reducer that sets initial query as both initial and current query in the state. But as I mentioned, having it as an argument also makes sense
Description
Screen.Recording.2025-06-17.at.9.48.19.PM.mov
https://jira.mongodb.org/browse/CLOUDP-325467
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes