refactor: migrate from XState to Zustand for state management #1042
Conversation
- Replace XState machine with Zustand store for simpler state management - Migrate feature highlighting functionality to Zustand - Migrate bounding box selection workflow to Zustand - Remove XState dependencies and xstate directory - All existing tests pass, including E2E bbox interaction test
- Add comprehensive test organization with shared utilities - Create test_utils.py with common helper functions and constants - Consolidate validation functions and interaction patterns - Add bbox selection workflow tests with proper state validation - Add feature highlighting stability tests - Improve test maintainability with self-explaining code - Organize tests by functionality (bbox vs feature highlighting) - Add shared fixtures in conftest.py for test reuse - Remove duplicate code patterns across test files - All 10 tests passing with improved coverage
…ration - Remove verbose comments and improve import organization in index.tsx - Clean up formatting in store.ts and toolbar.tsx for better readability - Add ClassVar type annotations to test constants for type safety - Improve code formatting and consistency in test files - Remove unused variables in test files for better maintainability
kylebarron
left a comment
kylebarron
left a comment
There was a problem hiding this comment.
Thanks for the PR!
We currently have two different places for state management:
- backbone models (via jupyter widgets) for any state coming in from Python
- xstate -> zustand for any "within JS application/UI state"
Do you think this split makes sense? Should we try to unify all state in a single place?
- Move state files to dedicated src/state/ directory for better organization - Clean up and standardize JSDoc comments across state management files - Add clear separation between client-side (Zustand) and Python-synced state - Improve inline comments in index.tsx for better code readability - Remove redundant documentation, rely on concise code comments
|
@kylebarron thanks for the review. I think we should keep them separate. The parts handled by Zustand don’t need to be exposed, this avoids unnecessary back-and-forth serialization for UI-only state. I added some changes to better document this approach. This is ready for another review. |
|
@kylebarron I added more test coverage to include the tooltip on feature hover. I won't be adding more changes to this until your feedback. |
There was a problem hiding this comment.
We have a lot of state in different places. Essentially the entire folder of src/model (https://github.com/developmentseed/lonboard/tree/main/src/model) is just definitions for state to be synced between Python and JS.
Do you intend for Zustand to be used with any of that? Would Zustand help at all beyond the current events that backbone propagates?
| if (isDrawingBbox) { | ||
| bboxProps.getFillColor = [255, 255, 0, 120]; | ||
| bboxProps.getLineColor = [211, 211, 38, 200]; | ||
| bboxProps.getLineWidth = 2; | ||
| } |
There was a problem hiding this comment.
These can always be set on bboxProps. The canonical way to toggle visibility is with the layer's visible prop. So we can set visible: false in general and then
if (isDrawingBbox) {
bboxProps.visible = true;
}
|
Sorry for the slow review! It's been a super busy month |
Co-authored-by: Kyle Barron <kyle@developmentseed.org>
Summary
Fixes #916