refactor(crud): update store to redux from reflux COMPASS-6844#8208
refactor(crud): update store to redux from reflux COMPASS-6844#8208Anemy wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the Compass CRUD plugin’s core store from Reflux to Redux (while explicitly keeping the grid store in Reflux for now), reshaping state management around reducer slices and wiring UI components to react-redux. Overall, the direction is sound for maintainability, but the scope is broad and there are a few correctness gaps that should be addressed before landing.
Changes:
- Introduces a Redux root reducer for CRUD and splits state into slice reducers (documents, collection-meta, view, insert, bulk update/delete) with thunk-based side effects.
- Updates CRUD UI components to use
connect/Redux actions and introduces a React context bridge for the still-Reflux grid store. - Updates and adds tests to validate slice reducers and the Redux-backed plugin store behavior.
Reviewed changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-crud/src/utils/parse-shell-bson.ts | Adds shared BSON parsing helper for bulk update parsing. |
| packages/compass-crud/src/utils/is-action.ts | Adds typed Redux action type guard helper. |
| packages/compass-crud/src/utils/fetch-documents.ts | Adds fetch/findAndModify helpers used by Redux thunks. |
| packages/compass-crud/src/stores/view.ts | Adds Redux slice for document view + table drilldown state. |
| packages/compass-crud/src/stores/reducer.ts | Adds Redux root reducer + shared action/thunk typing. |
| packages/compass-crud/src/stores/insert.ts | Adds Redux slice + thunks for insert dialog flows. |
| packages/compass-crud/src/stores/insert.spec.ts | Adds reducer unit tests for insert slice behavior. |
| packages/compass-crud/src/stores/grid-store-context.ts | Adds React context bridge for Reflux-backed grid store. |
| packages/compass-crud/src/stores/documents.ts | Adds Redux slice + thunks for fetching/paging/editing documents and emitting events. |
| packages/compass-crud/src/stores/crud-store.spec.ts | Refactors store tests to dispatch Redux actions/thunks and assert on Redux state. |
| packages/compass-crud/src/stores/collection-meta.ts | Adds Redux slice for collection metadata/stats. |
| packages/compass-crud/src/stores/bulk-update.ts | Adds Redux slice + thunks for bulk update modal + preview + execution toasts. |
| packages/compass-crud/src/stores/bulk-delete.ts | Adds Redux slice + thunk for bulk delete confirmation + execution toasts/events. |
| packages/compass-crud/src/plugin-title.tsx | Converts tab title header to connect to read collection stats from Redux. |
| packages/compass-crud/src/index.ts | Updates plugin provider to supply grid store via context; switches header/content to Redux-connected components. |
| packages/compass-crud/src/components/table-view/full-width-cell-renderer.tsx | Adjusts prop types away from legacy CrudActions signatures. |
| packages/compass-crud/src/components/table-view/document-table-view.tsx | Subscribes to grid store via context instead of passing the whole crud store. |
| packages/compass-crud/src/components/table-view/document-table-view.spec.tsx | Minor test typing cleanup. |
| packages/compass-crud/src/components/table-view/cell-renderer.tsx | Adjusts prop typing for drilldown callback signature. |
| packages/compass-crud/src/components/table-view/cell-editor.tsx | Adjusts prop typing and removes unnecessary RowNode casts. |
| packages/compass-crud/src/components/table-view/cell-editor.spec.tsx | Minor test typing cleanup. |
| packages/compass-crud/src/components/json-editor.tsx | Updates action prop types for Redux-dispatched functions. |
| packages/compass-crud/src/components/insert-document-dialog.tsx | Converts insert dialog to a connected component using Redux slice actions/thunks. |
| packages/compass-crud/src/components/insert-document-dialog.spec.tsx | Refactors tests to render dialog under a real activated Redux store. |
| packages/compass-crud/src/components/editable-document.tsx | Updates action prop types away from legacy CrudActions signatures. |
| packages/compass-crud/src/components/document-list.tsx | Converts main view to connect, dispatching Redux thunks; injects grid actions via context. |
| packages/compass-crud/src/components/crud-toolbar.tsx | Updates prop types for Redux-backed state (nullable count, numeric resultId). |
| packages/compass-crud/src/components/change-view/change-view.tsx | Small TS casting cleanup in change-view components. |
| packages/compass-crud/src/components/bulk-update-modal.tsx | Converts bulk update modal to connected wrapper and derives filter from query bar hook. |
| packages/compass-crud/src/components/bulk-update-modal.spec.tsx | Updates tests to import the unconnected modal component. |
| packages/compass-crud/src/components/bulk-delete-modal.tsx | Converts bulk delete modal to connected wrapper and derives filter from query bar hook. |
| packages/compass-crud/src/components/bulk-delete-modal.spec.tsx | Updates tests to import the unconnected modal component. |
| packages/compass-crud/src/actions/index.ts | Scopes Reflux actions down to grid-store-only actions. |
| packages/compass-crud/README.md | Removes outdated examples referencing Reflux CRUD actions. |
| packages/compass-crud/package.json | Removes reflux-state-mixin; adds redux/react-redux/redux-thunk dependencies. |
| packages/compass-collection/src/modules/collection-tab.ts | Adds isMockDataGeneratorEnabled metadata field used by CRUD options typing. |
| package-lock.json | Updates lockfile for dependency changes. |
gribnoysup
left a comment
gribnoysup
left a comment
There was a problem hiding this comment.
How do folks feel about having the grid store in another pr? Should we just do all of it at once?
I honestly feel like we should do this another way around and start from leaf stores, carefully evaluating the changes and what needs to be in their own slices (maybe their own stores / plugins) and how it is shaped, and only then move to the main crud store.
I left a few smaller comments based on a few first smaller store files, but the general problem with moving the current state and actions shape 1 to 1 from reflux to redux is that we are just inheriting all the current anti patterns of how this is done in a slightly different tech and I don't think this is moving us into the right direction
…tra event emit, typos
| 'Bulk Update Opened', | ||
| { | ||
| isUpdatePreviewSupported: | ||
| getState().collectionMeta.isUpdatePreviewSupported, |
There was a problem hiding this comment.
Actions should access these values from collection modal injected via locator, we duplicate values to state only if we need to drive UI updates with them
| await dispatch( | ||
| updateBulkUpdatePreview(updateText ?? INITIAL_BULK_UPDATE_TEXT) | ||
| ); | ||
| dispatch({ type: BulkUpdateActionTypes.OPEN_BULK_UPDATE }); |
There was a problem hiding this comment.
Batching actions is usually an anipattern, if you need some reusable functions, you can just write normal functions to use in multiple places, they don't need to be actions. In this particular case as we're blocking the modal from showing up until we get the preview, there's no reason for the state updates to happen before this
Also looking at show the updateBulkUpdatePreview is used in UI, this this also wrong: the actual action / event here is user changing text in the input, the preview update is just a side effect, and so the whole thing needs to be an action
|
|
||
| export type PreviewUpdatedAction = { | ||
| type: typeof BulkUpdateActionTypes.FETCH_PREVIEW_FINISHED; | ||
| updateText: string; |
There was a problem hiding this comment.
updateText being changed as part of this (or any other action where it exists currently) action doesn't make sense: if we need this value in store, we should probably have a proper action that indicates the update query being changed
…nd state, remove extra abort controller calls, better test setup crud store
| const query = queryBar.getLastAppliedQuery('crud'); | ||
|
|
||
| const { affected } = getState().bulkDelete; | ||
| dispatch(closeBulkDeleteDialog()); |
There was a problem hiding this comment.
So, once again, instead of reusing overly generic actions, I think it's really important that we get into the habit of actually modelling them after events happening in the app and not just firing setters everywhere as the redux documentation strongly recommends. Actual event here is that user started the delete (this closes the modal), followed up by us asking for confirmation, followed up by actually running the delete. You even actually have this part already implemented: "started" action closes the model, so why are we closing it with the setter here before instead of just dispatching the more meaningful action higher in the flow?
| collectionMeta: CollectionMetaState; | ||
| }) { | ||
| return combineReducers({ | ||
| documents: createDocumentsReducer(initial.documents), |
There was a problem hiding this comment.
I'm not sure where this is coming from, but redux provides its own API for passing in initial state, we don't need to invent our own instead of it
COMPASS-6844
This updates the base crud store to redux from reflux, and separates it out into a few reducers, insert, collection-meta, bulk-update, bulk-delete. This pr the does not update the grid-store, which is still in reflux, created COMPASS-10840 to track that.