Code Review Phase 4: Redux Store, Slices & Selectors

[jitu5]

Code Review Phase 4: Redux Store, Slices & Selectors

Purpose: Review the state management layer — the nervous system of the app. Every slice follows the same normalized { byId, allIds } pattern, and selectors use createSelector for memoization.

Prerequisites: Phases 1-3 (Types, Constants, Domain Layer)
Estimated time: 2 hours
Files: 20

---

Architecture Diagram

Redux Data Flow:

Component ──dispatch(action)──▶ Reducer ──▶ New State ──▶ Selector ──▶ Component
                                   │                        ▲
                                   │                        │
                                   ▼                   (memoized via
                            autoSaveMiddleware          createSelector)
                                   │
                                   ▼
                             localStorage

Normalized State Shape (each entity slice):
┌─────────────────────────────────┐
│ { byId: { "node-1": {...},     │  ◀── O(1) lookup by ID
│           "node-2": {...} },   │
│   allIds: ["node-1","node-2"], │  ◀── Iteration order
│   selected: ["node-1"] }       │  ◀── Multi-select tracking
└─────────────────────────────────┘

---

Files to Review (in order)

Store Setup

• src/store/hooks.ts — Typed useAppDispatch and useAppSelector hooks
• src/store/index.ts — Store configuration with 7 reducers + middleware
• src/store/middleware/autoSaveMiddleware.ts — Debounced auto-save to localStorage

Feature Slices

• src/features/project/projectSlice.ts — Project CRUD
• src/features/project/projectSelectors.ts — Project selectors
• src/features/nodes/nodesSlice.ts — Node CRUD + selection management
• src/features/nodes/nodesSelectors.ts — Memoized node selectors
• src/features/datasets/datasetsSlice.ts — Dataset CRUD + selection
• src/features/datasets/datasetsSelectors.ts — Memoized dataset selectors
• src/features/connections/connectionsSlice.ts — Connection CRUD
• src/features/connections/connectionsSelectors.ts — Connection selectors
• src/features/ui/uiSlice.ts — UI state (modals, panels, tutorial, walkthrough)
• src/features/ui/uiSelectors.ts — UI selectors
• src/features/validation/validationSlice.ts — Validation results storage
• src/features/validation/validationSelectors.ts — Validation selectors
• src/features/theme/themeSlice.ts — Theme toggle
• src/features/canvas/canvasSelectors.ts — Combined selectors with Set-based optimization

Tests

• src/features/nodes/nodesSlice.test.ts — 14 tests: node reducer mutations (add, update, delete, select)
• src/features/datasets/datasetsSlice.test.ts — 14 tests: dataset reducer mutations
• src/store/middleware/preferencesMiddleware.test.ts — 5 tests: auto-save middleware trigger/debounce behavior
• src/store/middleware/autoSaveMiddleware.test.ts — 3 tests: trigger, skip, and debounce behavior

---

Key Concepts

• Redux Toolkit's createSlice uses Immer under the hood, so "mutating" code like state.byId[id] = node is actually producing immutable updates
• The prepare callback in addNode/addDataset enables action creators to accept flexible inputs and normalize them before reaching the reducer
• createSelector from RTK memoizes selectors — they only recompute when input selectors return new references
• selectSelectedNodeIdsSet creates a Set from the array for O(1) lookups in hot render paths

---

Focus Areas

• Verify reducers are pure: uiSlice.ts no longer calls localStorage.setItem() directly — persistence is handled by autoSaveMiddleware. themeSlice.ts only reads localStorage at initialization time via safeGetItem(STORAGE_KEYS.THEME). Confirm no new side effects have crept in.
• Both uiSlice and themeSlice now use STORAGE_KEYS constants. Verify consistency.
• deleteNodes and deleteDatasets now use Set for O(n) complexity. Verify the Set-based implementation is correct.
• Are createSelector-based selectors properly memoized when called with inline parameters?
• autoSaveMiddleware uses module-level let saveTimeout — is this safe with hot module replacement?
• The theme selector was split out of the canvas mega-selector into its own standalone selector. Verify this doesn't break theme-dependent components.
• Review slice tests — do they cover edge cases like deleting non-existent nodes, selecting already-selected items, empty state operations?
• Review middleware tests — do they verify debounce timing and action filtering?

---

Next: Phase 5 (Persistence & Telemetry)

[ravi-kumar-pilla]

Some issues identified and possible fixes -

1. datasets/deleteDatasets Missing from SAVE_TRIGGER_ACTIONS - Data Loss Bug

File: src/store/middleware/autoSaveMiddleware.ts
Severity: Critical - data loss

The datasetsSlice exports a deleteDatasets action for bulk deletion, but autoSaveMiddleware's trigger list only covers the singular datasets/deleteDataset:

// autoSaveMiddleware.ts - current state
const SAVE_TRIGGER_ACTIONS = [
  'datasets/addDataset',
  'datasets/updateDataset',
  'datasets/updateDatasetPosition',
  'datasets/deleteDataset',     // ← singular only
  // 'datasets/deleteDatasets'  ← MISSING: bulk delete never triggers a save
];

The equivalent bulk actions for nodes (nodes/deleteNodes) and connections (connections/deleteConnections) are present. Only datasets are missing it. A user who multi-selects and deletes several datasets will have those deletions silently not persisted to localStorage until the next unrelated save-triggering action.

Fix: Add 'datasets/deleteDatasets' to SAVE_TRIGGER_ACTIONS.

Test gap: The autoSaveMiddleware.test.ts has no test exercising deleteDatasets, which masked this bug.

---

2. Parameterized Selectors Break Memoization - Performance Bug

Files:

• src/features/nodes/nodesSelectors.ts — selectNodeById
• src/features/datasets/datasetsSelectors.ts — selectDatasetById
• src/features/connections/connectionsSelectors.ts — selectConnectionById
• src/features/validation/validationSelectors.ts — selectNodeValidationStatus
• src/features/validation/validationSelectors.ts — selectDatasetValidationStatus

Severity: Critical - correctness and performance

All five selectors follow this pattern:

export const selectNodeById = createSelector(
  [selectNodesById, (_state: RootState, nodeId: string) => nodeId],
  (byId, nodeId) => byId[nodeId]
);

RTK's createSelector (built on Reselect) maintains a cache size of 1 by default. A single selector instance called with different nodeId values across multiple components will always be a cache miss — every call with a different argument invalidates the previous cache entry.

In practice: a pipeline canvas with 20 nodes where each <NodeComponent> calls selectNodeById(state, node.id) will produce 20 cache misses on every state change, recomputing every node lookup even when the byId map hasn't changed. The selector as written provides zero memoization benefit when called with different IDs.

The correct patterns are:

Option A — Factory function (most idiomatic):

export const makeSelectNodeById = (nodeId: string) =>
  createSelector(selectNodesById, (byId) => byId[nodeId]);
// Usage: const selectNode = useMemo(() => makeSelectNodeById(id), [id]);

Option B — RTK createSelector with lruMemoize and maxSize (RTK ≥ 2.0):

import { createSelectorCreator, lruMemoize } from '@reduxjs/toolkit';
const createLruSelector = createSelectorCreator(lruMemoize, { maxSize: 50 });
export const selectNodeById = createLruSelector(
  [selectNodesById, (_state: RootState, id: string) => id],
  (byId, id) => byId[id]
);

The same issue applies identically to selectNodeValidationStatus and selectDatasetValidationStatus in validationSelectors.ts, which are called per-component for every node/dataset on the canvas.

---

3. RootState Manually Defined Instead of Derived — Synchronization Risk

Files:

• src/types/redux.ts
• src/store/index.ts

Severity: High — type safety regression risk

The current pattern manually maintains a RootState interface in types/redux.ts and re-exports it from the store:

// types/redux.ts
export interface RootState {
  project: ProjectState;
  nodes: NodesState;
  // ... 5 more entries
}

// store/index.ts
import type { RootState as ReduxRootState } from '../types/redux';
export type RootState = ReduxRootState;  // re-export the manual definition

The RTK-idiomatic approach is to derive RootState directly from the store:

// store/index.ts
export type RootState = ReturnType<typeof store.getState>;

With the current pattern, adding a new slice to configureStore's reducer object without updating the interface in types/redux.ts will cause TypeScript to report the store's actual state as incompatible with RootState. Worse, if the manual interface is accidentally diverged in the other direction (has a field the store doesn't), selectors will compile fine but access undefined at runtime.

The two sources of truth are currently in sync (7 slices in both places), but this is a fragile invariant to maintain manually. The manual type in types/redux.ts should be replaced with the derived type from the store.

---

4. addNode Prepare Callback — Inconsistent ID Validation

Files:

• src/features/nodes/nodesSlice.ts
• src/features/datasets/datasetsSlice.ts

Severity: Medium — correctness bug

The addNode prepare callback checks only 'id' in payload to decide whether to use an existing ID or generate one:

// nodesSlice.ts
prepare: (payload: KedroNode | { type: NodeType; position: ... }) => {
  if ('id' in payload) {   // ← truthy for { id: '' } or { id: undefined }
    return { payload };
  }
  // generate new id...
}

The addDataset prepare callback correctly adds a truthiness check:

// datasetsSlice.ts
if ('id' in payload && payload.id) {  // ← correct: rejects empty/falsy IDs
  return { payload: payload as KedroDataset };
}

A call like dispatch(addNode({ id: '', type: 'data_processing', position: ... })) would store a node with an empty string key in byId, which would then fail lookups and break downstream selectors. The fix is to add && payload.id to the 'id' in payload check in nodesSlice.

---

5. selectSelectionType Returns 'nodes' for Empty Selection

File: src/features/canvas/canvasSelectors.ts
Severity: Medium — semantic bug, incorrect downstream behavior

export const selectSelectionType = createSelector(
  [selectSelectedNodeIds, selectSelectedEdgeIds],
  (nodeIds, edgeIds): 'nodes' | 'edges' | 'mixed' => {
    if (nodeIds.length > 0 && edgeIds.length > 0) return 'mixed';
    if (nodeIds.length > 0) return 'nodes';
    if (edgeIds.length > 0) return 'edges';
    return 'nodes';   // ← Bug: returns 'nodes' when nothing is selected
  }
);

When nothing is selected, this selector returns 'nodes' — a semantically incorrect value. Any component consuming this selector to drive behavior (e.g., keyboard shortcut context, toolbar state, delete action scope) will incorrectly believe node-mode selection is active. The return type should include 'none' and the empty case should return it, or the fallback should be explicitly documented as a deliberate default.

[ravi-kumar-pilla]

Other minor issues -

1. No Named Theme Selector — Inline State Access Scattered Across Components

Observed in: 4 component files
Pattern:

// ThemeToggle.tsx, App.tsx, SettingsModal.tsx, CodeDisplay.tsx
const theme = useAppSelector((state) => state.theme.theme);

Every other slice has a dedicated selector file. Theme has none — components access state.theme.theme directly via inline arrow functions. This means:

• Refactoring the theme state shape requires updating 4+ component files
• No memoization for derived theme values
• Inconsistent with the rest of the selector pattern in the codebase

A themeSelectors.ts file with export const selectTheme = (state: RootState) => state.theme.theme; is missing and should be created.

---

2. saveProjectToLocalStorage Silently Diverges updatedAt

File: src/infrastructure/localStorage/localStorage.ts

const projectData: StoredProjectState = {
  project: {
    ...state.project.current,
    updatedAt: Date.now(), // Update timestamp on save
  },
  ...
};

The serialized project.updatedAt written to localStorage is patched to Date.now() at write time without dispatching a Redux action. The result is that the in-memory Redux state (with the original updatedAt) and the persisted state (with the save time as updatedAt) silently diverge. When the project is reloaded, updatedAt will show the save time rather than the actual last-mutation time.

The setLastSaved(number) action and lastSaved field in ProjectState already exist for tracking when a save occurred. The infrastructure layer should not be patching project data on write; instead the auto-save middleware could dispatch setLastSaved(Date.now()) after a successful save.

---

[ravi-kumar-pilla]

Hi @jitu5 ,

Overall the state management looks good. There are some issues flagged by our friend :) and I could confirm them manually. It will be great to have a fix for atleast these - #25 (comment) as they seem straight forward fixes. Please let me know if this is not clear.

Thank you

[rashidakanchwala]

Love this slice-based setup overall — it’s easy to follow and review.
Two high-level cleanup suggestions:

• selectSavedProjects and selectLastSaved aren’t tied to a current feature (confirmed with @jitu5), so we can remove them for now to reduce unused surface area.
• uiSlice currently has a lot ; splitting it into smaller feature-focused slices would improve maintainability and clarity.