Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds client-side per-frame edit batching and persistence, dynamic-position support for geometry editing, a new useFrameEditing hook, msgpack-numpy packing, a PATCH /api/rooms/<room_id>/frames/<frame_id>/partial endpoint, store APIs to register dynamic positions and persist pending edits, and tests for partial updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User (Browser)
participant Store as App Store
participant Hook as useFrameEditing
participant Query as TanStack Query Cache
participant API as API Client
participant Server as Server
participant Storage as Frame Storage
UI->>Hook: perform transform (translate/rotate)
Hook->>Hook: compute new positions & centroid
Hook->>Query: optimistic update specific frame cache
Hook->>Store: setPendingFrameEdit(frameId, key, data)
Store->>Store: accumulate pending edits
UI->>Store: saveFrameEdits()
Store->>API: partialUpdateFrame(roomId, frameId, updates)
API->>API: packBinary(updates) → msgpack
API->>Server: PATCH /frames/{frameId}/partial (msgpack)
Server->>Server: unpack msgpack, merge updates into frame
Server->>Storage: write updated frame atoms
Server->>Storage: update trajectory mapping
Server-->>API: {success, frame_id, updated_keys}
API->>Query: confirm/refresh cache
Store->>Store: clearPendingFrameEdits()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/zndraw/app/frame_routes.py (3)
936-936: Move imports to the top of the file.Per coding guidelines, imports should always be at the top of the file. The
msgpack_numpy,asebytes, andnumpyimports at lines 936, 1002, and 1008 should be moved to the import section at the top.Add these imports at the top of the file (after line 12):
import msgpack_numpy as m import numpy as np from asebytes import decode as asebytes_decode, encode as asebytes_encodeThen remove the inline imports at lines 936, 1002, and 1008.
1069-1071: Uselog.exceptionfor cleaner error logging.
logging.exceptionautomatically includes the traceback, making the manualtraceback.format_exc()call unnecessary.except Exception as e: - log.error(f"Failed to partial update frame: {e}\n{traceback.format_exc()}") + log.exception(f"Failed to partial update frame: {e}") return {"error": f"Failed to partial update frame: {e}"}, 500
956-958: Consider removing hex dump debug logging before production.The hex dump of raw request bytes at line 957 is useful for debugging msgpack encoding issues, but could be removed or gated behind a more verbose debug flag before production deployment.
tests/test_partial_frame_update.py (3)
26-41: Add timeout to requests calls for CI stability.All
requests.post/requests.patchcalls lack timeouts. If the server hangs, tests will block indefinitely. Static analysis flagged this (S113).response = requests.post( - f"{server}/api/rooms/{room}/join", json={}, headers=auth_headers + f"{server}/api/rooms/{room}/join", json={}, headers=auth_headers, timeout=30 )Consider creating a helper or using a session with default timeout to avoid repetition across all test functions.
44-61: Minor: Document endianness assumption in dtype mapping.The dtype strings assume little-endian byte order (
<f8,<i4, etc.). This matches typical x86/x64 test environments but could cause issues on big-endian systems.Consider adding a brief comment:
def _get_numpy_dtype_string(arr: np.ndarray) -> str: """Get numpy dtype string in the format msgpack-numpy expects. Note: Assumes little-endian byte order for test simplicity. """
143-145: Prefix unused variable with underscore.Static analysis flagged
visas unused. Per Python convention:def test_partial_update_info(room_with_frames_and_lock): """Test that partial update successfully modifies info data.""" - server, room, session_id, auth_headers, vis = room_with_frames_and_lock + server, room, session_id, auth_headers, _vis = room_with_frames_and_lockapp/src/utils/geometryEditing.ts (1)
58-64: Add bounds check for incomplete position data.If
data.lengthis not divisible by 3 (e.g., truncated data), the last iteration could read past valid data or produce an incomplete position tuple.export function convertFloat32ToPositionArray(data: Float32Array): number[][] { const positions: number[][] = []; + // Ensure we only process complete position tuples + const validLength = Math.floor(data.length / 3) * 3; - for (let i = 0; i < data.length; i += 3) { + for (let i = 0; i < validLength; i += 3) { positions.push([data[i], data[i + 1], data[i + 2]]); } return positions; }app/src/store.tsx (1)
1101-1150: Consider race condition with rapid frame changes.The fire-and-forget auto-save pattern (line 1111) is appropriate for not blocking UI, but rapid frame changes could cause overlapping saves. If frame A's save is still in flight when frame B's save starts, they may complete out of order. This is likely acceptable since each targets a different frame, but consider adding a note or tracking in-flight saves if order matters.
Also, the hardcoded check for
"arrays.positions"(line 1118) may need extension if other frame data keys require shape information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
app/src/components/three/KeyboardShortcutsHandler.tsx(3 hunks)app/src/components/three/MultiGeometryTransformControls.tsx(7 hunks)app/src/components/three/Particles.tsx(4 hunks)app/src/hooks/useFrameEditing.ts(1 hunks)app/src/hooks/useGeometryEditing.ts(2 hunks)app/src/myapi/client.ts(2 hunks)app/src/store.tsx(9 hunks)app/src/utils/geometryEditing.ts(2 hunks)app/src/utils/msgpack-numpy.ts(2 hunks)pyproject.toml(1 hunks)src/zndraw/app/frame_routes.py(1 hunks)tests/test_partial_frame_update.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for your classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Use type hints wherever possible. Useimport typing as tif necessary, but preferlist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
src/zndraw/app/frame_routes.pytests/test_partial_frame_update.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test_*.py: Usepytest.mark.parametrizeto avoid code duplication in tests
Each test must be a function, not a method of a class
Files:
tests/test_partial_frame_update.py
🧬 Code graph analysis (5)
app/src/myapi/client.ts (1)
app/src/utils/msgpack-numpy.ts (1)
packBinary(492-498)
src/zndraw/app/frame_routes.py (2)
src/zndraw/app/redis_keys.py (2)
RoomKeys(241-479)trajectory_indices(338-340)src/zndraw/app/frame_index_manager.py (3)
FrameIndexManager(12-283)append(35-55)get_all(235-244)
app/src/components/three/MultiGeometryTransformControls.tsx (1)
app/src/utils/geometryEditing.ts (3)
LoadedPositionData(348-352)validateEditablePositions(363-408)calculateCombinedCentroidWithDynamic(420-493)
app/src/store.tsx (1)
app/src/myapi/client.ts (1)
partialUpdateFrame(713-736)
tests/test_partial_frame_update.py (3)
src/zndraw/zndraw.py (1)
ZnDraw(367-1805)tests/conftest.py (1)
get_jwt_auth_headers(130-132)tests/test_room_lock_enforcement.py (1)
auth_headers(38-40)
🪛 Biome (2.1.2)
app/src/utils/msgpack-numpy.ts
[error] 381-381: Shouldn't redeclare 'TypedArrayConstructor'. Consider to delete it or rename it.
'TypedArrayConstructor' is defined here:
(lint/suspicious/noRedeclare)
🪛 Ruff (0.14.8)
src/zndraw/app/frame_routes.py
911-911: Unused function argument: session_id
(ARG001)
911-911: Unused function argument: user_id
(ARG001)
1069-1069: Do not catch blind exception: Exception
(BLE001)
1070-1070: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/test_partial_frame_update.py
26-26: Probable use of requests call without timeout
(S113)
33-33: Probable use of requests call without timeout
(S113)
114-114: Probable use of requests call without timeout
(S113)
145-145: Unpacked variable vis is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
154-154: Probable use of requests call without timeout
(S113)
202-202: Probable use of requests call without timeout
(S113)
226-226: Probable use of requests call without timeout
(S113)
253-253: Probable use of requests call without timeout
(S113)
274-274: Probable use of requests call without timeout
(S113)
299-299: Probable use of requests call without timeout
(S113)
309-309: Probable use of requests call without timeout
(S113)
338-338: Probable use of requests call without timeout
(S113)
368-368: Probable use of requests call without timeout
(S113)
419-419: Probable use of requests call without timeout
(S113)
464-464: Probable use of requests call without timeout
(S113)
471-471: Probable use of requests call without timeout
(S113)
482-482: Probable use of requests call without timeout
(S113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
- GitHub Check: pytest (3.12, ubuntu-latest)
🔇 Additional comments (37)
app/src/utils/msgpack-numpy.ts (3)
415-439: Implementation looks correct for numpy format encoding.The function correctly creates the msgpack-numpy dict format with
nd,type,shape, anddatakeys. TheUint8Arrayview at line 437 properly captures the raw bytes from the typed array's buffer.
448-482: Recursive preparation logic is well-structured.The function correctly handles all data types: primitives pass through, TypedArrays get converted to numpy format with optional shape lookup, and nested objects/arrays are recursively processed.
492-498: Clean composition of prepare and encode steps.The
packBinaryexport correctly orchestrates the conversion pipeline. The function signature with optionalshapesMap provides flexibility for specifying array dimensions.app/src/hooks/useGeometryEditing.ts (2)
116-120: Early return for dynamic positions is correctly implemented.The explicit
nullcheck (vsundefined) properly distinguishes between "dynamic positions handled elsewhere" and "positions not yet loaded". This allowsuseFrameEditingto manage dynamic positions without this hook interfering.
353-371: Dependency array updated correctly.Adding
positionDatato the dependency array ensures the effect re-initializes when position data changes, which is necessary for the new early return logic to work properly.app/src/components/three/KeyboardShortcutsHandler.tsx (2)
324-329: Save shortcut implementation is clean and follows existing patterns.The
Skey binding for saving frame edits in editing mode mirrors the existingTkey handler pattern withvoidfor fire-and-forget async execution and early return.
320-322: Async handling updated correctly.Adding
voidprefix and explicitreturnensures the asynccycleTransformModeis properly fire-and-forget without blocking or causing unhandled promise warnings.src/zndraw/app/frame_routes.py (3)
959-973: Decode hook correctly handles JS/Python key format differences.The
decode_with_string_keyshook properly identifies numpy array markers with string keys from JavaScript and converts them to byte keys before passing tomsgpack-numpy. This enables seamless interoperability between the TypeScriptpackBinaryand Python decoder.
911-911: Unused arguments are required by the decorator pattern.The
session_idanduser_idparameters are injected by the@requires_lockdecorator after lock validation. This is consistent with other endpoints in this file (e.g.,delete_frames_batch,append_frame). The static analysis warning can be safely ignored.
1018-1026: > Likely an incorrect or invalid review comment.pyproject.toml (1)
15-15: Dependency version bump is in use but changelog unavailable for verification.The
asebytes>=0.1.7requirement is correctly used inframe_routes.pywhereasebytes.decode,asebytes.encode, andasebytes.get_metadataare called for partial frame updates. However, asebytes 0.1.7 was recently released (2025-12-13) and has no publicly available changelog or GitHub release notes to confirm what changes or features justify bumping from 0.1.6. Verify with the asebytes maintainers if this version bump introduces necessary fixes or API changes required by the new functionality.app/src/myapi/client.ts (1)
697-736: LGTM! Well-structured partial update API function.The implementation correctly:
- Uses
packBinaryfor msgpack encoding with optional shapes- Wraps in
Blobto ensure exact byte count (good practice for binary payloads)- Sets appropriate
Content-Type: application/msgpackheaderOne minor consideration: the function doesn't have specific error handling for common failure cases (404, 423 lock errors). The caller will need to handle these appropriately.
app/src/hooks/useFrameEditing.ts (2)
185-212: Good optimistic cache update pattern.The dual cache update approach (specific key + broader frame data) ensures consistency across different query patterns. Using
useAppStore.getState()inside the callback correctly avoids stale closure issues.
221-236: VerifytransformModedependency is intentional.
transformModeis in the dependency array but is only accessed viagetState()inside the callback. This causes the effect to re-run (and re-subscribe) when transform mode changes.If this re-initialization is unintended, consider removing it from deps since the callback already reads the latest value via
getState(). If intentional (e.g., to reset state on mode change), the current implementation is correct.tests/test_partial_frame_update.py (1)
404-455: Excellent data integrity test.This test properly verifies that partial updates only modify the targeted keys while preserving other frame data (numbers, cell). This is critical for preventing data corruption.
app/src/utils/geometryEditing.ts (3)
72-82: LGTM. Internal utility with expected contract.The function assumes positions are valid
[x, y, z]tuples. Since this is an internal utility where callers control the input format, this is acceptable.
363-408: Well-structured validation logic.The three-way categorization (static valid, dynamic with loaded data, invalid) cleanly separates concerns and provides useful information to callers about why selections may be invalid.
420-493: Correct combined centroid calculation.The function properly:
- Processes both static (
number[][]) and dynamic (Float32Array) positions- Validates indices against bounds before accessing
- Returns
nullfor empty selections (avoiding division by zero)app/src/components/three/MultiGeometryTransformControls.tsx (3)
76-87: Good memoization of loaded positions map.Converting the store's
loadedDynamicPositionsto theMap<string, LoadedPositionData>format expected by utilities is correctly memoized. The dependency onloadedDynamicPositionsensures the map is rebuilt when dynamic positions change.
155-168: Correct integration of dynamic position support.The validation and centroid calculation now properly handle both static and dynamic positions by:
- Using
validateEditablePositionswhich returns bothvalidSelectionsanddynamicSelections- Passing both selection types to
calculateCombinedCentroidWithDynamicThis ensures transform controls work correctly with mixed static/dynamic geometry selections.
191-198: LGTM. Dependency array is complete.The effect correctly depends on
loadedPositionsMapto re-calculate centroid when dynamic position data changes.app/src/components/three/Particles.tsx (6)
8-9: LGTM!New imports for frame editing functionality are correctly added.
102-107: LGTM!Store selectors for dynamic position registration follow the established pattern.
387-394: LGTM!Dynamic position detection and data extraction is correctly implemented with proper null safety.
396-438: LGTM!The dual-effect pattern for registration and cleanup is well-designed:
- Using a ref to track the registered key avoids stale closure issues in the cleanup effect
- The comment explains the intentional gap-avoidance during refetch
- Dependency arrays are correct
441-450: LGTM!Correctly separates static and dynamic position handling by passing
nulltouseGeometryEditingfor dynamic positions.
452-459: LGTM!The
useFrameEditinghook is correctly wired up for dynamic positions, with an empty string key to disable it for static positions.app/src/store.tsx (10)
15-15: LGTM!Import for
partialUpdateFrameAPI function correctly added.
105-116: LGTM!State design for frame editing is well-structured with clear documentation for
loadedDynamicPositions.
314-317: LGTM!Initial state correctly initialized for all new frame editing fields.
847-868: LGTM!Correctly saves pending frame edits before exiting editing mode and resets the
isEditingFrameDataflag.
879-894: LGTM!Correctly saves pending frame edits before cycling to the next transform mode.
1152-1152: LGTM!Simple and correct implementation.
1154-1197: LGTM!Good implementation with proper validation, error handling that preserves pending edits for retry, and correct API usage matching the
partialUpdateFramesignature fromclient.ts.
1199-1199: LGTM!Simple and correct implementation.
1201-1211: LGTM!Correct immutable update pattern for Map state in Zustand - creating a new Map ensures React detects the state change.
154-154: All callers properly handle the async signature.The
cycleTransformModefunction is correctly defined as async (line 154) with implementation at line 879 that awaitssaveFrameEdits()before cycling. The only caller inKeyboardShortcutsHandler.tsx(line 320) properly handles the promise using thevoidoperator with an explicit "fire and forget" comment, preventing any unhandled promise rejections.
There was a problem hiding this comment.
Pull request overview
This PR implements dynamic editing functionality that allows users to edit frame data (such as atomic positions) loaded from the server, rather than only static geometry data. The implementation adds a new partial frame update endpoint on the backend and comprehensive frontend infrastructure to handle dynamic position editing with real-time updates.
Key Changes:
- New backend endpoint for partial frame updates with msgpack encoding/decoding support
- Frontend hook system (
useFrameEditing) for managing edits to dynamic positions - Integration with TanStack Query cache for optimistic updates and instant visual feedback
- Support for batch editing and auto-save functionality on mode changes
- Comprehensive test suite covering various edge cases and error conditions
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Updated asebytes dependency from 0.1.6 to 0.1.7 |
| pyproject.toml | Updated asebytes requirement to >=0.1.7 |
| src/zndraw/app/frame_routes.py | Added partial_update_frame endpoint for merging updates with existing frame data |
| tests/test_partial_frame_update.py | Comprehensive test suite for the new endpoint covering success cases and error conditions |
| app/src/utils/msgpack-numpy.ts | Added packBinary function to encode TypedArrays in msgpack-numpy format |
| app/src/utils/geometryEditing.ts | Added utilities for validating and transforming dynamic positions |
| app/src/store.tsx | Added frame editing state management, pending edits tracking, and save functionality |
| app/src/myapi/client.ts | Added partialUpdateFrame API client function |
| app/src/hooks/useFrameEditing.ts | New hook for managing dynamic position editing with optimistic cache updates |
| app/src/hooks/useGeometryEditing.ts | Updated to skip validation when positions are handled by useFrameEditing |
| app/src/components/three/Particles.tsx | Integrated useFrameEditing hook and dynamic position registration |
| app/src/components/three/MultiGeometryTransformControls.tsx | Updated to support both static and dynamic positions in centroid calculation |
| app/src/components/three/KeyboardShortcutsHandler.tsx | Added 'S' key shortcut for manual save of frame edits |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if frame_id < 0 or frame_id >= frame_count: | ||
| return { | ||
| "error": f"Invalid frame index {frame_id}, valid range: 0-{frame_count - 1}", | ||
| "type": "IndexError", | ||
| }, 404 |
There was a problem hiding this comment.
The endpoint validates that frame_id is not negative by checking frame_id >= 0, but negative frame indices could still reach this point if Flask's routing changes. While the test documents that Flask's route pattern doesn't match negative numbers (returning 405), relying on Flask's routing behavior for validation is fragile. Consider adding explicit validation that frame_id >= 0 with a clear error message, or documenting this as an assumption about Flask's behavior.
| } | ||
| return m.decode(byte_key_obj) | ||
| # Fall through to default msgpack-numpy decode for byte keys | ||
| return m.decode(obj) |
There was a problem hiding this comment.
The decode_with_string_keys function is defined inside the request handler, but it's calling m.decode recursively which can lead to infinite recursion. When msgpack-numpy's decode function encounters a dict without the 'nd' marker, it won't convert it and will return the original object. However, if that object is passed to the object_hook again, it creates a loop.
The function should return obj (not m.decode(obj)) for the fallthrough case to let msgpack-numpy's normal decoding handle it.
| return m.decode(obj) | |
| return obj |
| // Mark that we're editing frame data | ||
| setIsEditingFrameData(true); | ||
|
|
||
| // Check if combined centroid has changed | ||
| const centroidKey = editingCombinedCentroid.join(","); | ||
| const centroidChanged = lastCombinedCentroidRef.current !== centroidKey; | ||
|
|
||
| // Initialize or re-initialize if centroid changed | ||
| if (centroidChanged) { | ||
| const centroid = editingCombinedCentroid; | ||
| initialCentroidRef.current = centroid; | ||
|
|
||
| // Store initial positions for transform calculations | ||
| initialPositionsRef.current = new Float32Array(positionData); | ||
|
|
||
| // Convert to number[][] for utility functions | ||
| const positionsArray = convertFloat32ToPositionArray(positionData); | ||
|
|
||
| // Calculate relative positions using the combined centroid | ||
| const relative = getRelativePositions( | ||
| positionsArray, | ||
| stableSelectedIndices, | ||
| centroid, | ||
| ); | ||
| relativePositionsRef.current = relative; | ||
|
|
||
| // Store this centroid to detect future changes | ||
| lastCombinedCentroidRef.current = centroidKey; | ||
| } | ||
|
|
||
| // Now subscribe (refs are guaranteed to be set at this point) | ||
| if ( | ||
| !initialCentroidRef.current || | ||
| relativePositionsRef.current.size === 0 || | ||
| !initialPositionsRef.current | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| // Subscribe to transform changes from MultiGeometryTransformControls | ||
| const unsubscribe = subscribeToEditing( | ||
| geometryKey, | ||
| (matrix: THREE.Matrix4) => { | ||
| // Handle translate and rotate modes (positions only) | ||
| // Rotation is orbital rotation around centroid - positions change, not object rotation | ||
| // (particles are spheres, so they don't have individual rotation) | ||
| // Scale mode is not supported for dynamic positions | ||
| const currentTransformMode = useAppStore.getState().transformMode; | ||
| if ( | ||
| currentTransformMode !== "translate" && | ||
| currentTransformMode !== "rotate" | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| // Get current refs | ||
| const currentCentroid = initialCentroidRef.current; | ||
| const currentRelativePositions = relativePositionsRef.current; | ||
| const currentInitialPositions = initialPositionsRef.current; | ||
|
|
||
| if ( | ||
| !currentCentroid || | ||
| currentRelativePositions.size === 0 || | ||
| !currentInitialPositions | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| // Convert initial positions to number[][] format | ||
| const initialPositionsArray = convertFloat32ToPositionArray( | ||
| currentInitialPositions, | ||
| ); | ||
|
|
||
| // Apply transform to get new positions | ||
| const newPositionsArray = applyTransformToPositions( | ||
| initialPositionsArray, | ||
| stableSelectedIndices, | ||
| matrix, | ||
| currentCentroid, | ||
| currentRelativePositions, | ||
| ); | ||
|
|
||
| // Convert back to Float32Array | ||
| const newPositions = convertPositionArrayToFloat32(newPositionsArray); | ||
|
|
||
| // Update TanStack Query cache optimistically | ||
| // This immediately updates all components using this data (including bonds) | ||
| const currentRoomId = useAppStore.getState().roomId; | ||
| const frame = useAppStore.getState().currentFrame; | ||
|
|
||
| if (currentRoomId) { | ||
| // Update the specific key in the frame response cache | ||
| const queryKey = ["frame", currentRoomId, frame, positionKey]; | ||
| queryClient.setQueryData(queryKey, newPositions); | ||
|
|
||
| // Also update any cache that might have the full frame response | ||
| // This ensures consistency across different query patterns | ||
| queryClient.setQueriesData( | ||
| { queryKey: ["frame", currentRoomId, frame] }, | ||
| (oldData: any) => { | ||
| if (oldData && typeof oldData === "object") { | ||
| return { | ||
| ...oldData, | ||
| [positionKey]: newPositions, | ||
| }; | ||
| } | ||
| return oldData; | ||
| }, | ||
| ); | ||
|
|
||
| // Register this as a pending edit for batch persistence | ||
| setPendingFrameEdit(frame, positionKey, newPositions); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| return () => { | ||
| unsubscribe(); | ||
| // Reset flag when this geometry stops editing frame data | ||
| setIsEditingFrameData(false); | ||
| }; |
There was a problem hiding this comment.
The useFrameEditing hook calls setIsEditingFrameData(true) inside an effect, but the cleanup only resets it to false. If multiple geometries use this hook simultaneously (e.g., multiple particle systems with dynamic positions), the flag will incorrectly flip between true and false as different components mount/unmount or update. Consider using a reference counting mechanism or set of active geometry keys instead of a simple boolean flag.
| dtype = arr.dtype | ||
| # Map common dtypes to msgpack-numpy format | ||
| dtype_map = { | ||
| np.float64: "<f8", | ||
| np.float32: "<f4", | ||
| np.int64: "<i8", | ||
| np.int32: "<i4", | ||
| np.int16: "<i2", | ||
| np.int8: "|i1", | ||
| np.uint64: "<u8", | ||
| np.uint32: "<u4", | ||
| np.uint16: "<u2", | ||
| np.uint8: "|u1", | ||
| np.bool_: "|b1", | ||
| } | ||
| return dtype_map.get(dtype.type, str(dtype)) |
There was a problem hiding this comment.
The helper function _get_numpy_dtype_string uses a dictionary mapping type objects to dtype strings, but it uses dtype.type which returns the numpy scalar type. This approach works but is fragile - if numpy changes how dtype.type works, this mapping could break. Consider using dtype.str (the standard numpy dtype string representation) directly instead of maintaining a custom mapping, or add tests to verify this mapping stays correct.
| dtype = arr.dtype | |
| # Map common dtypes to msgpack-numpy format | |
| dtype_map = { | |
| np.float64: "<f8", | |
| np.float32: "<f4", | |
| np.int64: "<i8", | |
| np.int32: "<i4", | |
| np.int16: "<i2", | |
| np.int8: "|i1", | |
| np.uint64: "<u8", | |
| np.uint32: "<u4", | |
| np.uint16: "<u2", | |
| np.uint8: "|u1", | |
| np.bool_: "|b1", | |
| } | |
| return dtype_map.get(dtype.type, str(dtype)) | |
| return arr.dtype.str |
| # Append the new frame data to physical storage | ||
| new_physical_index = len(storage) | ||
| storage.append(updated_frame) | ||
|
|
||
| # Update the frame mapping to point to new physical index | ||
| # Get the original score to preserve gap-based indexing | ||
| frame_mapping_with_scores = index_manager.get_all(withscores=True) | ||
| _, old_score = frame_mapping_with_scores[frame_id] | ||
|
|
||
| # Remove old mapping and add new one | ||
| pipeline = r.pipeline() | ||
| pipeline.zrem(room_keys.trajectory_indices(), mapping_entry) | ||
| pipeline.zadd( | ||
| room_keys.trajectory_indices(), | ||
| {f"{room_id}:{new_physical_index}": old_score}, | ||
| ) | ||
| pipeline.execute() |
There was a problem hiding this comment.
The backend partial_update_frame function always creates a new physical frame entry and updates the mapping, even for small changes like updating a single position. This means every edit creates a new entry in storage without cleaning up old ones. Over many edits, this could lead to storage bloat. Consider implementing a garbage collection mechanism or documenting this behavior as a known limitation.
| useGeometryEditing( | ||
| geometryKey, | ||
| finalPositionData, | ||
| positionIsDynamic ? null : finalPositionData, // Only pass if static |
There was a problem hiding this comment.
The comment states "Only pass if static" but the logic passes null for dynamic positions. This creates an implicit contract where null means "skip this hook for dynamic positions". This is error-prone because null typically means "no data" rather than "handled elsewhere". Consider adding a more explicit parameter like isDynamic or skipDynamicHandling to make the intent clearer.
| // Use frame editing for dynamic positions (string references) | ||
| useFrameEditing( | ||
| geometryKey, | ||
| positionIsDynamic && typeof positionProp === "string" ? positionProp : "", |
There was a problem hiding this comment.
The condition checks if positionProp is a string twice - once in the ternary and once in the type guard. The second check typeof positionProp === "string" is redundant because if positionIsDynamic is true and isPositionDynamic returns true for strings, then positionProp must be a string. This redundant check could be removed or simplified to just use positionIsDynamic.
| positionIsDynamic && typeof positionProp === "string" ? positionProp : "", | |
| positionIsDynamic ? positionProp : "", |
| def test_partial_update_empty_body(room_with_frames_and_lock): | ||
| """Test that partial update returns 400 for empty update body.""" | ||
| server, room, session_id, auth_headers, _ = room_with_frames_and_lock | ||
|
|
||
| # Send empty dict | ||
| update_data = msgpack.packb({}) | ||
|
|
||
| response = requests.patch( | ||
| f"{server}/api/rooms/{room}/frames/0/partial", | ||
| data=update_data, | ||
| headers={ | ||
| **auth_headers, | ||
| "X-Session-ID": session_id, | ||
| "Content-Type": "application/msgpack", | ||
| }, | ||
| ) | ||
|
|
||
| assert response.status_code == 400 | ||
| result = response.json() | ||
| assert "No keys provided" in result["error"] |
There was a problem hiding this comment.
The test uses msgpack.packb({}) to send an empty dictionary, but this creates a valid msgpack payload. The endpoint checks if len(unpacked_data) == 0, so this test is correctly verifying the empty keys validation. However, consider also adding a test case for completely invalid/malformed msgpack data to ensure proper error handling for corrupted requests.
| import { | ||
| decode as msgpackDecode, | ||
| encode as msgpackEncode, | ||
| ExtensionCodec, |
There was a problem hiding this comment.
Unused import ExtensionCodec.
| ExtensionCodec, |
| """ | ||
|
|
||
| import msgpack | ||
| import numpy as np |
There was a problem hiding this comment.
Import of 'm' is not used.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #804 +/- ##
==========================================
- Coverage 77.80% 77.71% -0.09%
==========================================
Files 147 148 +1
Lines 17760 18008 +248
==========================================
+ Hits 13818 13995 +177
- Misses 3942 4013 +71 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/zndraw/app/frame_routes.py (1)
1046-1065: Consider documenting the copy-on-write storage pattern and its implications.The partial update always creates a new physical frame entry (line 1050-1051) and updates the mapping to point to it. While this preserves frame history and simplifies concurrency, it means:
- Every edit creates a new storage entry, potentially leading to storage bloat over many edits
- Old frame data is never garbage collected
- Cross-room frame references remain safe (editing doesn't mutate shared data)
This is a reasonable design choice for an MVP, but consider:
- Documenting this behavior in the endpoint docstring
- Planning future work for garbage collection of unreferenced frames
- Possibly adding a compaction/cleanup endpoint for managing storage
Based on past review comments, this concern has already been raised. The current implementation is acceptable for the initial release.
tests/test_partial_frame_update.py (1)
44-61: Test helper works correctly but could be simplified.The
_get_numpy_dtype_stringfunction usesdtype.typewith a manual mapping, which works but is somewhat fragile. As noted in past comments, usingdtype.strdirectly would be simpler and more maintainable. However, since this is test-only code and the current implementation works correctly, this is a low-priority refactor.The current implementation is acceptable for test purposes.
app/src/store.tsx (1)
1103-1152: Auto-save pattern has acceptable tradeoffs but document the async behavior.The fire-and-forget auto-save (lines 1113-1138) when switching frames is a pragmatic design choice that prioritizes UI responsiveness:
✅ Pros:
- Non-blocking: user can immediately edit the new frame
- Notifications: snackbar alerts on auto-save success/failure
- Most common case: saves succeed quickly
⚠️ Tradeoffs:
- Race condition: old save could complete after new save starts
- Failed auto-saves don't block the user, but data could be lost
- No retry mechanism for failed auto-saves
Consider adding a JSDoc comment documenting this behavior, so future maintainers understand the async implications.
+ /** + * Set a pending edit for a frame key. + * + * If there are pending edits for a different frame, they will be auto-saved + * asynchronously (fire-and-forget) before accumulating the new edit. + * This is a non-blocking operation to keep the UI responsive. + */ setPendingFrameEdit: (frameId, key, data) => {Based on past review comments, this concern has been previously raised.
🧹 Nitpick comments (4)
app/src/utils/msgpack-numpy.ts (2)
401-425: Document the shape parameter requirement for multi-dimensional arrays.The default shape calculation at line 414 creates a 1D array
[arr.byteLength / typedArr.BYTES_PER_ELEMENT], which won't correctly represent multi-dimensional data like positions (which should be[N, 3]). While the docstring mentions that shapes should be provided, consider adding a warning log or throwing an error whenshapeis undefined and the data is likely multi-dimensional, to prevent silent data corruption.// Get BYTES_PER_ELEMENT from the typed array const typedArr = arr as unknown as { BYTES_PER_ELEMENT: number }; - const arrayShape = shape || [arr.byteLength / typedArr.BYTES_PER_ELEMENT]; + const arrayShape = shape || (() => { + const flatLength = arr.byteLength / typedArr.BYTES_PER_ELEMENT; + console.warn( + `[typedArrayToNumpyFormat] No shape provided, defaulting to 1D array [${flatLength}]. ` + + `For multi-dimensional arrays, provide explicit shape parameter.` + ); + return [flatLength]; + })();
434-468: Document limitation: shapes parameter doesn't support nested array structures.The
prepareForPackfunction recursively builds keyPaths like"key[0]"for array elements (line 452), but theshapesMap expects keys without array indices (e.g.,"arrays.positions"). This means if you have nested arrays containing TypedArrays, the shape lookup will fail.Based on the PR context, the intended use case is top-level frame data keys like
"arrays.positions", so this limitation may be acceptable. However, document this clearly in thepackBinaryfunction's JSDoc to prevent misuse./** * Pack data into msgpack format with numpy array support. * Converts TypedArrays to the msgpack-numpy dict format that Python can decode. * * @param data - Data to pack (can contain TypedArrays) - * @param shapes - Optional map of key paths to shapes (e.g., {"arrays.positions": [100, 3]}) + * @param shapes - Optional map of key paths to shapes (e.g., {"arrays.positions": [100, 3]}). + * Note: Only supports top-level keys, not nested array paths like "key[0].subkey". * @returns Msgpack-encoded Uint8Array */src/zndraw/app/frame_routes.py (1)
1081-1083: Uselog.exceptionfor automatic traceback logging.Replace
log.errorwithlog.exceptionto automatically include the stack trace in the error log, which aids debugging without the manualtraceback.format_exc()call.except Exception as e: - log.error(f"Failed to partial update frame: {e}\n{traceback.format_exc()}") + log.exception(f"Failed to partial update frame: {e}") return {"error": f"Failed to partial update frame: {e}"}, 500Based on coding guidelines for Python.
tests/test_partial_frame_update.py (1)
1-516: Consider adding timeouts to test HTTP requests.While not critical for tests against a local server, adding timeout parameters to all
requests.*calls would prevent tests from hanging indefinitely if the server becomes unresponsive. This is a best practice for test reliability.Example:
response = requests.post( f"{server}/api/rooms/{room}/join", json={}, headers=auth_headers, timeout=10, # 10 second timeout )Apply this pattern to all requests calls in the test file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/components/three/Particles.tsx(4 hunks)app/src/hooks/useFrameEditing.ts(1 hunks)app/src/store.tsx(9 hunks)app/src/utils/msgpack-numpy.ts(2 hunks)src/zndraw/app/frame_routes.py(1 hunks)tests/test_partial_frame_update.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for your classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Use type hints wherever possible. Useimport typing as tif necessary, but preferlist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
tests/test_partial_frame_update.pysrc/zndraw/app/frame_routes.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test_*.py: Usepytest.mark.parametrizeto avoid code duplication in tests
Each test must be a function, not a method of a class
Files:
tests/test_partial_frame_update.py
🧬 Code graph analysis (5)
app/src/hooks/useFrameEditing.ts (1)
app/src/utils/geometryEditing.ts (4)
convertFloat32ToPositionArray(58-64)getRelativePositions(128-148)applyTransformToPositions(159-210)convertPositionArrayToFloat32(72-82)
app/src/store.tsx (1)
app/src/myapi/client.ts (1)
partialUpdateFrame(713-736)
tests/test_partial_frame_update.py (3)
src/zndraw/zndraw.py (1)
ZnDraw(367-1805)tests/conftest.py (2)
server(261-267)s22(303-305)tests/test_room_lock_enforcement.py (1)
auth_headers(38-40)
src/zndraw/app/frame_routes.py (5)
src/zndraw/app/route_utils.py (4)
requires_lock(76-219)parse_frame_mapping(547-573)get_storage(44-73)emit_frames_invalidate(336-378)src/zndraw/app/redis_keys.py (3)
session_id(515-517)RoomKeys(241-479)trajectory_indices(338-340)src/zndraw/storage/asebytes_backend.py (1)
get(46-103)src/zndraw/storage/base.py (2)
get(17-35)append(77-79)src/zndraw/frame_cache.py (2)
items(42-43)keys(10-11)
app/src/components/three/Particles.tsx (2)
app/src/utils/geometryEditing.ts (1)
isPositionDynamic(26-28)app/src/hooks/useFrameEditing.ts (1)
useFrameEditing(41-244)
🪛 Ruff (0.14.8)
tests/test_partial_frame_update.py
26-26: Probable use of requests call without timeout
(S113)
33-33: Probable use of requests call without timeout
(S113)
114-114: Probable use of requests call without timeout
(S113)
145-145: Unpacked variable vis is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
154-154: Probable use of requests call without timeout
(S113)
202-202: Probable use of requests call without timeout
(S113)
226-226: Probable use of requests call without timeout
(S113)
253-253: Probable use of requests call without timeout
(S113)
274-274: Probable use of requests call without timeout
(S113)
299-299: Probable use of requests call without timeout
(S113)
309-309: Probable use of requests call without timeout
(S113)
338-338: Probable use of requests call without timeout
(S113)
368-368: Probable use of requests call without timeout
(S113)
419-419: Probable use of requests call without timeout
(S113)
464-464: Probable use of requests call without timeout
(S113)
471-471: Probable use of requests call without timeout
(S113)
482-482: Probable use of requests call without timeout
(S113)
504-504: Probable use of requests call without timeout
(S113)
src/zndraw/app/frame_routes.py
911-911: Unused function argument: session_id
(ARG001)
911-911: Unused function argument: user_id
(ARG001)
1081-1081: Do not catch blind exception: Exception
(BLE001)
1082-1082: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.12, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
🔇 Additional comments (13)
app/src/utils/msgpack-numpy.ts (1)
470-484: LGTM! Clean public API for msgpack-numpy encoding.The
packBinaryfunction provides a clean interface for encoding mixed data with TypedArrays. The implementation correctly:
- Prepares TypedArrays into numpy format via
prepareForPack- Encodes the result with standard msgpack
- Accepts optional shape metadata for multi-dimensional arrays
This pairs well with the existing
unpackBinaryfunction for round-trip encoding/decoding.src/zndraw/app/frame_routes.py (2)
965-990: LGTM! Correct handling of JavaScript vs Python msgpack-numpy format differences.The
decode_numpy_recursivelyfunction correctly handles the key encoding mismatch between JavaScript (string keys like'nd') and Python msgpack-numpy (byte keys likeb'nd'). The implementation:
- Detects numpy array markers with either string or byte keys
- Converts string keys to bytes for msgpack-numpy decoding
- Recursively processes nested structures without infinite loops
- Returns primitives unchanged
This bridges the gap between the TypeScript
packBinary(which uses string keys) and Python's msgpack-numpy decoder.
1019-1042: Solid implementation of partial update key handling.The update application logic correctly:
- Parses dotted key format (
arrays.positions,info.energy,calc.forces)- Converts values to numpy arrays with
np.asarrayfor consistency- Defensively creates a
SinglePointCalculatorwhen updating calc results if none exists- Logs warnings for unknown key formats
This provides a clean interface for partial updates while maintaining ASE data structure conventions.
tests/test_partial_frame_update.py (1)
98-456: Excellent test coverage for the partial update endpoint.The test suite comprehensively covers:
- ✅ Happy path: positions, info, multiple keys
- ✅ Edge cases: different frame indices, empty rooms, empty bodies
- ✅ Error handling: invalid indices, missing locks, missing session IDs, malformed msgpack
- ✅ Data integrity: preservation of non-updated fields, frame isolation
- ✅ Round-trip verification with fresh client connections
The tests verify both the API contract and the underlying data persistence, providing strong confidence in the implementation.
app/src/hooks/useFrameEditing.ts (3)
86-221: Correct use of reference counting for concurrent editing sessions.The hook properly manages multiple simultaneous editing instances using
incrementEditingFrameDataCount()/decrementEditingFrameDataCount()instead of a simple boolean flag. This prevents premature cleanup when one geometry component unmounts while others are still editing.The implementation correctly:
- Increments the counter when entering valid editing state (line 102)
- Decrements in the cleanup function (line 220)
- Uses the store's ref-count to derive whether any editing is active
This addresses the race condition concern from past reviews.
186-213: Query cache update pattern is correct for the current query structure.The optimistic cache update (lines 193-209) correctly:
- Updates the specific key query:
["frame", roomId, frame, positionKey]- Updates any full-frame queries with a predicate matcher
The broad predicate pattern at line 199 matches
["frame", currentRoomId, frame]prefix, which could theoretically match unintended queries. However, based on the codebase's query key structure (individual keys use 4-element tuples), this is safe.If the query key structure changes in the future, ensure this predicate is updated accordingly.
Based on analysis of the codebase's query patterns.
15-21: Good performance optimization with array equality check.Using a dedicated
arraysEqualfunction instead ofJSON.stringifyfor comparingselectedIndicesis a smart optimization. This avoids expensive serialization and provides O(n) comparison, improving re-render performance for frequently changing selections.app/src/components/three/Particles.tsx (3)
402-431: Correct registration ordering prevents data gaps during updates.The dynamic position registration logic (lines 411-421) correctly registers the new geometry key BEFORE unregistering the old one, preventing brief gaps where dynamic position data would be unavailable. This is especially important during:
- Geometry key changes
- Data refetches
- Component re-mounts
The ref-based tracking ensures cleanup happens with the correct key on unmount.
This addresses past review feedback about registration ordering.
443-462: Clean separation between static and dynamic editing paths.The hook integration correctly:
- Passes
nulltouseGeometryEditingfor dynamic positions, allowing the hook to skip handling- Passes position data to
useFrameEditingonly for dynamic positions- Uses conditional logic to route between the two editing modes
The type cast at line 458 is technically redundant (TypeScript knows
positionPropis a string whenpositionIsDynamicis true) but aids readability. The empty string fallback for the non-dynamic case is safe sinceuseFrameEditingcheckspositionData !== nullbefore processing.
434-440: Proper cleanup prevents memory leaks in dynamic position tracking.The unmount cleanup effect correctly uses
registeredGeometryKeyRef.currentto unregister the dynamic position data. Using a ref ensures the cleanup uses the actual registered key even if the component's props changed before unmount, preventing orphaned entries in the store'sloadedDynamicPositionsmap.app/src/store.tsx (3)
105-117: Well-designed state structure for frame editing.The new state fields provide:
pendingFrameEdits: Batches multiple key updates for a single frame before persistence, reducing API callseditingFrameDataCount: Reference counter enabling multiple components to edit simultaneously without conflictsloadedDynamicPositions: Centralized registry of dynamic position data for centroid calculations and validationThe typing is precise, and the structure supports the copy-on-write editing pattern implemented in the backend.
1156-1199: Clean implementation of frame edit persistence.The
saveFrameEditsaction correctly:
- ✅ Validates preconditions (roomId, pendingFrameEdits)
- ✅ Handles empty edits gracefully (early return)
- ✅ Builds shape metadata for
arrays.positions(Nx3 format)- ✅ Calls the partial update API with proper error handling
- ✅ Clears pending edits only on success
- ✅ Provides user feedback via snackbar
The hard-coded shape inference for positions (
[numAtoms, 3]) is correct for the current use case. If additional multi-dimensional arrays are added in the future, this logic would need extension.
849-863: Async mode transitions prevent data loss during editing.The changes to make
exitEditingModeandcycleTransformModeasync (returningPromise<void>) enable proper sequencing:
- exitEditingMode (line 850): Saves pending frame edits before releasing the lock and clearing selections, preventing loss of unsaved work
- cycleTransformMode (lines 882-883): Saves pending edits before switching modes, ensuring consistency
This is defensive programming that improves data integrity. Callers (like
KeyboardShortcutsHandler) already handle these as async operations, so the signature change is breaking but coordinated.Also applies to: 881-896
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.