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 5 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
| export const BulkDeleteActionTypes = { | ||
| OPEN_DIALOG: 'crud/bulk-delete/OPEN_DIALOG', | ||
| CLOSE_DIALOG: 'crud/bulk-delete/CLOSE_DIALOG', | ||
| IN_PROGRESS: 'crud/bulk-delete/IN_PROGRESS', | ||
| } as const; |
There was a problem hiding this comment.
Actions should be closer to user triggered events, not just generic actions
There was a problem hiding this comment.
Updated these names, how does it look now?
There was a problem hiding this comment.
Naming is only part of the problem here, the "close" action is still just a setter basically, it's not an "event" that happens in the app that causes the state change eventually, we should aim for having a clear separation between those because ultimately the goal is to have a clear log of actual events happening in the app. If you have a generic "close" action both for start of the delete and for user just closing the modal, you completely lose this distinction. This is the important conceptual difference that we should really try to capture in the code even if it feels redundant to have the same reducer logic for two different actions. Same applies to the update slice
There was a problem hiding this comment.
Once again want to point out this doc https://redux.js.org/style-guide#model-actions-as-events-not-setters and suggest to look closely at the detailed explanation, it's obviously exagerated, but the same thinking applies
| import { refreshDocuments } from './documents'; | ||
|
|
||
| export type BulkDeleteState = { | ||
| previews: Document[]; |
There was a problem hiding this comment.
The line can be blurry sometimes, but we usually need a strong, good reason to store non-serializable instances like that in the store. Is there a good reason for it here? We serialize it at least once on open already, so why create instances here?
| PREVIEW_UPDATED: 'crud/bulk-update/PREVIEW_UPDATED', | ||
| PREVIEW_SYNTAX_ERROR: 'crud/bulk-update/PREVIEW_SYNTAX_ERROR', | ||
| PREVIEW_SERVER_ERROR: 'crud/bulk-update/PREVIEW_SERVER_ERROR', | ||
| RUN_AFFECTED_LATCHED: 'crud/bulk-update/RUN_AFFECTED_LATCHED', |
There was a problem hiding this comment.
Ditto very generic actions
There was a problem hiding this comment.
Updated these names, how does it look now?
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.
Opening in draft as there are still a few more things I want to move about and rename, and I want to do a bit more of a thorough review/think about myself.
How do folks feel about having the grid store in another pr? Should we just do all of it at once?