refactor: replace AsebytesStorage + StorageRouter with FrameStorage#877
Merged
refactor: replace AsebytesStorage + StorageRouter with FrameStorage#877
Conversation
Replace AsebytesStorage + StorageRouter with thin FrameStorage registry. All I/O through AsyncBlobIO public API. Zero _backend access. Selective key loading at call sites (isosurface, frames, result backend). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add AsyncBlobIO API reference table (row vs column access patterns) - Use column view API for result backend (io[b"_"][0] not io.get(keys=)) - Document why provider dispatch doesn't need keys passthrough - Document why reserve()/remove_items() are dead code (provider uses Redis) - Fix create_room write guard (checks source room, not target) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add design principle: prefer io[...] subscript over method calls - Document multi-key access: io[[b"k1", b"k2"]][i], .to_list(), .to_dict() - Expand API reference with column, multi-key, and mutation sections - Update all examples to use subscript (isosurface, result backend, batch) - Add "when to use which" decision guide - Update success criteria to require subscript API throughout Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add OOB index handling note (IndexError vs None, routes pre-validate) - Add explicit create_room before/after (two deps -> one, subscript API) - Add trajectory.py isinstance removal example - Add update_room frame_count patch clarification - Note slice view extra len() call overhead - Note build_room_update callers must update in same PR Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
14 tasks across 5 chunks: core FrameStorage class, route migrations, test migrations, cleanup and verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
FrameStorage is a thin room-scoped AsyncBlobIO registry with provider frame count support. No I/O wrappers - routes will use AsyncBlobIO subscript API directly. - Create storage/frame_storage.py with FrameStorage class - Update dependencies.py: FrameStorageDep + RequireWritableDep - Update database.py: direct FrameStorage init - Update result_backends.py: io[b"_"][0] column view - Delete asebytes_backend.py and router.py Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- frames.py: subscript reads/writes, RequireWritableDep on mutations - isosurface.py: subscript frame load - trajectory.py: subscript batch reads, remove isinstance check - rooms.py: single FrameStorageDep, subscript copy/extend - step.py, server_settings.py, socketio.py: FrameStorageDep swap Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace AsebytesStorage/StorageRouter/get_storage with FrameStorage/get_frame_storage across all test files. Storage calls now use the AsyncBlobIO subscript API (storage[room_id].extend, storage[room_id][i], etc.) instead of wrapper methods. - Create tests/test_frame_storage.py (replaces test_storage_asebytes.py) - Rewrite tests/test_storage_router.py for FrameStorage provider metadata - Update all integration test fixtures in 7 files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AsyncBlobIO raises IndexError for OOB indices (unlike old AsebytesStorage which returned None). Provider-backed rooms have frames declared in Redis but not yet materialized in storage, so reads at valid provider indices hit OOB in the actual backend. Use io.get(index) with try/except IndexError for provider-dispatch paths in get_frame, get_frame_metadata, and get_isosurface. For list_frames with sparse indices, pre-filter to in-bounds indices before to_list(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the AsebytesStorage/StorageRouter wrappers with a new FrameStorage registry exposing per-room AsyncBlobIO via a subscript API; updates DI (FrameStorageDep, RequireWritableDep), app lifespan, result backend, routes, and tests; removes old storage modules and adds FrameStorage unit tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant RouteHandler
participant RequireWritableDep
participant FrameStorageDep
participant FrameStorage
participant AsyncBlobIO
participant Redis
rect rgba(200,150,255,0.5)
Note over Client,Redis: Write flow (permission enforced)
Client->>RouteHandler: POST append_frames(room_id, frames)
RouteHandler->>RequireWritableDep: require_writable_room(room_id)
RequireWritableDep->>FrameStorageDep: get_frame_storage(request)
FrameStorageDep->>FrameStorage: return instance
RequireWritableDep->>Redis: check provider frame-count key
alt provider-backed (read-only)
RequireWritableDep-->>RouteHandler: raise RoomReadOnly
RouteHandler-->>Client: 409 / RoomReadOnly
else writable
RequireWritableDep-->>RouteHandler: allow
RouteHandler->>FrameStorage: storage[room_id]
FrameStorage->>AsyncBlobIO: return per-room IO
RouteHandler->>AsyncBlobIO: .extend(frames)
AsyncBlobIO-->>RouteHandler: ack
RouteHandler-->>Client: 200
end
end
rect rgba(150,200,255,0.5)
Note over Client,AsyncBlobIO: Read flow
Client->>RouteHandler: GET frame(room_id, index)
RouteHandler->>FrameStorageDep: get_frame_storage(request)
FrameStorageDep->>FrameStorage: return instance
RouteHandler->>FrameStorage: storage[room_id]
FrameStorage->>AsyncBlobIO: return per-room IO
RouteHandler->>AsyncBlobIO: [index] or [start:stop]
AsyncBlobIO-->>RouteHandler: frame(s) or IndexError
RouteHandler-->>Client: 200 / 404
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #877 +/- ##
==========================================
- Coverage 91.48% 91.37% -0.11%
==========================================
Files 180 179 -1
Lines 17466 17292 -174
==========================================
- Hits 15978 15800 -178
- Misses 1488 1492 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/zndraw/result_backends.py (1)
99-137:⚠️ Potential issue | 🟠 MajorDon't memoize provider-result hashes in
FrameStorage.Lines 120, 128, and 137 route arbitrary provider cache keys through
FrameStorage.__getitem__(), which memoizes every key in_rooms. Becausesrc/zndraw/database.pynow shares one app-wide registry here, even cache misses leave anAsyncBlobIOhandle behind until shutdown. With hashed provider-result keys, normal cache churn turns into unbounded registry growth. Use an ephemeral accessor for result-cache I/O, or add a publicdiscard()/open_ephemeral()path and call it after each operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/result_backends.py` around lines 99 - 137, The code currently calls FrameStorage.__getitem__ via self._storage[key] in store, get, and delete which memoizes keys in FrameStorage._rooms and leaks AsyncBlobIO handles; replace those direct __getitem__ uses with a non-memoizing ephemeral accessor (e.g. self._storage.open_ephemeral(key)) or, if such API doesn't exist, add and call a public open_ephemeral()/discard() pair on FrameStorage so each operation opens an ephemeral AsyncBlobIO, uses it (clear/extend/len/__getitem__), and then disposes/discards the handle; update the store, get, and delete methods to use that ephemeral open/discard pattern (or call discard immediately after each operation) instead of keeping references via __getitem__.
🧹 Nitpick comments (3)
src/zndraw/database.py (1)
238-243: HoistFrameStorageto the module import block.Line 239 runs during every lifespan startup and doesn't look like a lazy-load path. Moving it next to the other imports keeps the dependency graph easier to follow.
As per coding guidelines, "Import statements at top of file unless they affect startup time (lazy load)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/database.py` around lines 238 - 243, The FrameStorage import and instantiation should be moved out of the lifespan block: add "from zndraw.storage import FrameStorage" to the module-level imports and initialize app.state.frame_storage where app startup config is assembled (using settings.storage and app.state.redis) rather than inside the lifespan startup code; update or remove the local import at the former site and ensure the module-level import is used when creating app.state.frame_storage (referencing FrameStorage, app.state.frame_storage, settings.storage, and app.state.redis).tests/test_trajectory.py (1)
62-73: MoveAsyncMockimport to module top-level.The function-local import at line 65 violates the guideline that import statements should be at the top of the file unless they affect startup time.
AsyncMockhas no startup-time reason to be lazy-loaded.♻️ Proposed cleanup
import io from collections.abc import AsyncIterator from typing import Any +from unittest.mock import AsyncMock @@ async def traj_storage_fixture() -> AsyncIterator[FrameStorage]: """Create a fresh FrameStorage instance for each test.""" - from unittest.mock import AsyncMock - mock_redis = AsyncMock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_trajectory.py` around lines 62 - 73, The local import of AsyncMock inside traj_storage_fixture should be moved to the module top-level: add "from unittest.mock import AsyncMock" at the top of the test module and remove the function-local import; leave the rest of the fixture (mock_redis setup and FrameStorage("memory://", mock_redis) yield) unchanged so traj_storage_fixture continues to return the same AsyncMock-based mock Redis instance.docs/superpowers/plans/2026-03-18-storage-refactor.md (1)
914-922: Minor documentation inconsistency with actual implementation.The plan suggests replacing
storage.reserve(room.id, 3)withstorage.set_frame_count(room.id, 3), but the actual test implementation intest_frames_provider_dispatch.pyusesstorage[room.id].reserve(3)(callingAsyncBlobIO.reserve()directly). Both approaches are valid for simulating provider-backed rooms with empty slots, but the plan doesn't match the implemented approach.This is informational only — the implementation is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-03-18-storage-refactor.md` around lines 914 - 922, The docs refer to using reserve() to simulate provider mounts but the implementation and tests use AsyncBlobIO.reserve via storage[room.id].reserve(3); update the plan text to match the implementation by replacing the example that calls reserve() with the set_frame_count API (use set_frame_count(room.id, 3)) or, if you prefer to keep the test pattern, explicitly note that storage[room.id].reserve(3) (AsyncBlobIO.reserve) is an equivalent valid approach and show that symbol as an acceptable alternative; ensure you reference both set_frame_count() and AsyncBlobIO.reserve/storage[room.id].reserve in the updated paragraph so the plan matches the code in test_frames_provider_dispatch.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/zndraw/storage/frame_storage.py`:
- Around line 20-31: The to_raw_frame function currently decodes every str value
as base64; change it to only base64-decode values that are explicitly marked
(e.g., keys that start with "b64:"), leave unmarked strings unchanged (convert
plain str values to bytes via UTF-8 or keep them as non-encoded metadata per
RawFrame contract), and normalize keys to bytes; specifically update the logic
in to_raw_frame to: if a key is a str and startswith "b64:", strip that marker,
base64-decode the corresponding value (value must be str), if a value is a str
but key has no "b64:" marker then encode it with .encode("utf-8") (or wrap as
metadata according to RawFrame), and ensure keys are converted to bytes (e.g.,
key.encode("utf-8") if key is str); also update the docstring and any checks in
to_raw_frame to reflect the explicit "b64:" marker protocol and validate
decoding errors.
- Around line 134-152: The get_length method currently checks io.len() first
which can undercount for provider-mounted rooms; change the logic to read the
mounted provider count from Redis first via
self._redis.get(RedisKey.provider_frame_count(room_id)) and, if that value
exists, return it (int); only if no cached provider_frame_count is present
should you call io.len() on self[room_id] and return that result. Ensure you
still handle missing/None cached values and convert the cached string to int
before returning.
In `@tests/test_lifespan.py`:
- Around line 90-94: The test method test_memory_storage_creates_frame_storage
is defined as a class method; convert it to a module-level pytest function by
moving the def out of any test class (remove any leading self parameter) and
keep the body intact: instantiate AsyncMock(), create
FrameStorage(uri="memory://", redis=mock_redis) and assert isinstance(...,
FrameStorage). Ensure imports (AsyncMock and FrameStorage) remain available at
module scope and the function name stays
test_memory_storage_creates_frame_storage so pytest will discover it.
---
Outside diff comments:
In `@src/zndraw/result_backends.py`:
- Around line 99-137: The code currently calls FrameStorage.__getitem__ via
self._storage[key] in store, get, and delete which memoizes keys in
FrameStorage._rooms and leaks AsyncBlobIO handles; replace those direct
__getitem__ uses with a non-memoizing ephemeral accessor (e.g.
self._storage.open_ephemeral(key)) or, if such API doesn't exist, add and call a
public open_ephemeral()/discard() pair on FrameStorage so each operation opens
an ephemeral AsyncBlobIO, uses it (clear/extend/len/__getitem__), and then
disposes/discards the handle; update the store, get, and delete methods to use
that ephemeral open/discard pattern (or call discard immediately after each
operation) instead of keeping references via __getitem__.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-03-18-storage-refactor.md`:
- Around line 914-922: The docs refer to using reserve() to simulate provider
mounts but the implementation and tests use AsyncBlobIO.reserve via
storage[room.id].reserve(3); update the plan text to match the implementation by
replacing the example that calls reserve() with the set_frame_count API (use
set_frame_count(room.id, 3)) or, if you prefer to keep the test pattern,
explicitly note that storage[room.id].reserve(3) (AsyncBlobIO.reserve) is an
equivalent valid approach and show that symbol as an acceptable alternative;
ensure you reference both set_frame_count() and
AsyncBlobIO.reserve/storage[room.id].reserve in the updated paragraph so the
plan matches the code in test_frames_provider_dispatch.py.
In `@src/zndraw/database.py`:
- Around line 238-243: The FrameStorage import and instantiation should be moved
out of the lifespan block: add "from zndraw.storage import FrameStorage" to the
module-level imports and initialize app.state.frame_storage where app startup
config is assembled (using settings.storage and app.state.redis) rather than
inside the lifespan startup code; update or remove the local import at the
former site and ensure the module-level import is used when creating
app.state.frame_storage (referencing FrameStorage, app.state.frame_storage,
settings.storage, and app.state.redis).
In `@tests/test_trajectory.py`:
- Around line 62-73: The local import of AsyncMock inside traj_storage_fixture
should be moved to the module top-level: add "from unittest.mock import
AsyncMock" at the top of the test module and remove the function-local import;
leave the rest of the fixture (mock_redis setup and FrameStorage("memory://",
mock_redis) yield) unchanged so traj_storage_fixture continues to return the
same AsyncMock-based mock Redis instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8267aa89-000c-43e9-a9c3-2dd1ce11cc14
📒 Files selected for processing (27)
docs/superpowers/plans/2026-03-18-storage-refactor.mddocs/superpowers/specs/2026-03-17-storage-refactor-design.mdsrc/zndraw/database.pysrc/zndraw/dependencies.pysrc/zndraw/result_backends.pysrc/zndraw/routes/frames.pysrc/zndraw/routes/isosurface.pysrc/zndraw/routes/rooms.pysrc/zndraw/routes/server_settings.pysrc/zndraw/routes/step.pysrc/zndraw/routes/trajectory.pysrc/zndraw/socketio.pysrc/zndraw/storage/__init__.pysrc/zndraw/storage/asebytes_backend.pysrc/zndraw/storage/frame_storage.pysrc/zndraw/storage/router.pytests/test_constraints.pytests/test_frame_storage.pytests/test_frames_provider_dispatch.pytests/test_isosurface.pytests/test_lifespan.pytests/test_result_backends.pytests/test_routes_frames.pytests/test_routes_step.pytests/test_storage_asebytes.pytests/test_storage_router.pytests/test_trajectory.py
💤 Files with no reviewable changes (3)
- src/zndraw/storage/router.py
- src/zndraw/storage/asebytes_backend.py
- tests/test_storage_asebytes.py
Comment on lines
+20
to
+31
| def to_raw_frame(frame: dict[str, Any] | RawFrame) -> RawFrame: | ||
| """Convert input frame dict to raw bytes format. | ||
|
|
||
| Handles two input formats: | ||
| 1. Already raw: dict[bytes, bytes] -- pass through | ||
| 2. Base64 encoded: dict[str, str] with keys like "b64:..." -- decode | ||
|
|
||
| Parameters | ||
| ---------- | ||
| frame | ||
| Input frame data (dict[str, Any] or dict[bytes, bytes]). | ||
| """ |
There was a problem hiding this comment.
Don't treat every string value as base64.
The public API here accepts dict[str, Any], but Line 53 unconditionally decodes every str value as base64. Any ordinary string field will either be mangled or fail decoding. This needs an explicit marker/protocol for encoded values, with plain strings packed separately.
Also applies to: 52-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/zndraw/storage/frame_storage.py` around lines 20 - 31, The to_raw_frame
function currently decodes every str value as base64; change it to only
base64-decode values that are explicitly marked (e.g., keys that start with
"b64:"), leave unmarked strings unchanged (convert plain str values to bytes via
UTF-8 or keep them as non-encoded metadata per RawFrame contract), and normalize
keys to bytes; specifically update the logic in to_raw_frame to: if a key is a
str and startswith "b64:", strip that marker, base64-decode the corresponding
value (value must be str), if a value is a str but key has no "b64:" marker then
encode it with .encode("utf-8") (or wrap as metadata according to RawFrame), and
ensure keys are converted to bytes (e.g., key.encode("utf-8") if key is str);
also update the docstring and any checks in to_raw_frame to reflect the explicit
"b64:" marker protocol and validate decoding errors.
Member
Author
There was a problem hiding this comment.
Intentional. Values arriving through this path are always base64-encoded binary blobs from the frontend JSON API (msgpack-numpy data). Keys use the b64: marker because they can be either plain strings or encoded bytes, but values are always binary. No plain string values exist in the frame protocol. This code is unchanged from the original AsebytesStorage.
There was a problem hiding this comment.
@PythonFZ, understood — thanks for the clarification. Given that values arriving via this path are contractually always base64-encoded binary blobs from the frontend JSON API (msgpack-numpy encoded data), the unconditional decoding is correct. I'll note this protocol for future reviews.
✏️ Learnings added
Learnt from: PythonFZ
Repo: zincware/ZnDraw PR: 877
File: src/zndraw/storage/frame_storage.py:20-31
Timestamp: 2026-03-19T10:02:10.740Z
Learning: In `src/zndraw/storage/frame_storage.py`, the `to_raw_frame` function unconditionally base64-decodes all `str` values. This is intentional: values arriving through the frontend JSON API path are always base64-encoded binary blobs (msgpack-numpy data). Keys use the `b64:` marker to distinguish plain string keys from encoded bytes keys, but values are always binary — no plain string values exist in the ZnDraw frame protocol. This behavior is preserved from the original `AsebytesStorage` implementation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+134
to
+152
| async def get_length(self, room_id: str) -> int: | ||
| """Get frame count for a room. | ||
|
|
||
| Checks the AsyncBlobIO length first, then falls back to the | ||
| Redis provider frame count key (set by mounted providers). | ||
|
|
||
| Parameters | ||
| ---------- | ||
| room_id | ||
| Room identifier. | ||
| """ | ||
| io = self[room_id] | ||
| length = await io.len() | ||
| if length > 0: | ||
| return length | ||
| cached = await self._redis.get( # type: ignore[misc] | ||
| RedisKey.provider_frame_count(room_id) | ||
| ) | ||
| return int(cached) if cached else 0 |
There was a problem hiding this comment.
Mounted rooms should prefer Redis frame counts over io.len().
For provider-backed rooms, io.len() is only the locally materialized subset. Once a few frames are cached, callers like room_join() and /step can start reporting the cache size instead of the provider's advertised total. Read provider_frame_count first and only fall back to io.len() when no mount metadata exists.
💡 One safe way to keep mounted counts authoritative
async def get_length(self, room_id: str) -> int:
"""Get frame count for a room.
@@
- io = self[room_id]
- length = await io.len()
- if length > 0:
- return length
cached = await self._redis.get( # type: ignore[misc]
RedisKey.provider_frame_count(room_id)
)
- return int(cached) if cached else 0
+ if cached is not None:
+ return int(cached)
+ return await self[room_id].len()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/zndraw/storage/frame_storage.py` around lines 134 - 152, The get_length
method currently checks io.len() first which can undercount for provider-mounted
rooms; change the logic to read the mounted provider count from Redis first via
self._redis.get(RedisKey.provider_frame_count(room_id)) and, if that value
exists, return it (int); only if no cached provider_frame_count is present
should you call io.len() on self[room_id] and return that result. Ensure you
still handle missing/None cached values and convert the cached string to int
before returning.
Member
Author
There was a problem hiding this comment.
The current order is intentional and matches the original StorageRouter.get_length(). Provider-backed rooms use reserve() to pre-allocate all slots, so io.len() returns the full provider count. Real frames from extend() take precedence over the Redis provider count — this is validated by test_get_length_returns_storage_count. Checking Redis first would break that invariant.
There was a problem hiding this comment.
@PythonFZ, thanks for the detailed clarification — that's a key design invariant I missed. Since reserve() pre-allocates all slots upfront for provider-backed rooms, io.len() already reflects the full provider frame count, and real materialized frames from extend() naturally take precedence. Checking Redis first would indeed break that invariant. The current ordering is correct.
✏️ Learnings added
Learnt from: PythonFZ
Repo: zincware/ZnDraw PR: 877
File: src/zndraw/storage/frame_storage.py:134-152
Timestamp: 2026-03-19T10:02:21.264Z
Learning: In `src/zndraw/storage/frame_storage.py` (ZnDraw project), `FrameStorage.get_length()` intentionally checks `io.len()` (AsyncBlobIO length) **before** the Redis `provider_frame_count` key. Provider-backed rooms call `reserve()` to pre-allocate all slots in the AsyncBlobIO, so `io.len()` already returns the full provider frame count. Real frames written via `extend()` must take precedence over the Redis provider count. Reversing the order (Redis first) would break this invariant. This is validated by the `test_get_length_returns_storage_count` test.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+90
to
+94
| def test_memory_storage_creates_frame_storage(self) -> None: | ||
| """memory:// URI should create FrameStorage backend.""" | ||
| mock_redis = AsyncMock() | ||
| backend = FrameStorage(uri="memory://", redis=mock_redis) | ||
| assert isinstance(backend, FrameStorage) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify class-based pytest tests in this file.
rg -nP '^\s*class\s+Test' tests/test_lifespan.py
rg -nP '^\s{4}def\s+test_' tests/test_lifespan.pyRepository: zincware/ZnDraw
Length of output: 753
Convert this to a module-level pytest test function.
The test at line 90 is defined as a class method, which violates the guideline: tests/**/*.py requires tests to be functions, not class methods.
♻️ Suggested fix
-class TestStorageInitialization:
- """Test storage backend initialization based on config."""
-
- def test_memory_storage_creates_frame_storage(self) -> None:
- """memory:// URI should create FrameStorage backend."""
- mock_redis = AsyncMock()
- backend = FrameStorage(uri="memory://", redis=mock_redis)
- assert isinstance(backend, FrameStorage)
+def test_memory_storage_creates_frame_storage() -> None:
+ """memory:// URI should create FrameStorage backend."""
+ mock_redis = AsyncMock()
+ backend = FrameStorage(uri="memory://", redis=mock_redis)
+ assert isinstance(backend, FrameStorage)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_lifespan.py` around lines 90 - 94, The test method
test_memory_storage_creates_frame_storage is defined as a class method; convert
it to a module-level pytest function by moving the def out of any test class
(remove any leading self parameter) and keep the body intact: instantiate
AsyncMock(), create FrameStorage(uri="memory://", redis=mock_redis) and assert
isinstance(..., FrameStorage). Ensure imports (AsyncMock and FrameStorage)
remain available at module scope and the function name stays
test_memory_storage_creates_frame_storage so pytest will discover it.
Member
Author
There was a problem hiding this comment.
Pre-existing pattern — the other tests in this file (TestLifespanWithoutRedisUrl, TestLifespanWithRedisUrl, TestFakeRedisServer) are also class-based. Refactoring the whole file is out of scope for this PR.
There was a problem hiding this comment.
@PythonFZ, understood — the new test is consistent with the existing class-based pattern in the file, so it makes sense to leave it as-is for this PR. Would you like me to open a follow-up issue to track converting all tests in tests/test_lifespan.py to module-level functions in a future PR?
🧠 Learnings used
Learnt from: CR
Repo: zincware/ZnDraw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-06T10:07:24.874Z
Learning: Applies to tests/**/*.py : Use `pytest.mark.parametrize` for tests. Each test must be a function, not a class method
Learnt from: CR
Repo: zincware/ZnDraw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-06T10:07:24.874Z
Learning: Applies to tests/**/*.py : Write specific tests that test one thing. Avoid complex setups
…is with real Redis in tests upload_trajectory was missing write-protection for provider-backed rooms after the StorageRouter removal. All test FrameStorage fixtures now use the shared conftest frame_storage fixture backed by real Redis instead of AsyncMock. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/zndraw/routes/trajectory.py (2)
33-39:⚠️ Potential issue | 🟡 MinorDeclare the new read-only problem response on this endpoint.
Line 242 adds
RequireWritableDep, soupload_trajectorycan now fail withRoomReadOnly, but the decorator still omits that branch fromproblem_responses(...). The handler will still work, but the OpenAPI contract will miss this Problem JSON response.Proposed fix
from zndraw.exceptions import ( InvalidPayload, NotAuthenticated, RoomLocked, + RoomReadOnly, RoomNotFound, problem_responses, ) @@ "", status_code=status.HTTP_201_CREATED, responses=problem_responses( - NotAuthenticated, RoomNotFound, RoomLocked, InvalidPayload + NotAuthenticated, RoomNotFound, RoomLocked, RoomReadOnly, InvalidPayload ), )As per coding guidelines: "Use RFC 9457 Problem JSON (application/problem+json) for all 4xx/5xx REST API responses".
Also applies to: 230-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/routes/trajectory.py` around lines 33 - 39, The endpoint's OpenAPI problem responses omit the new read-only error branch: add RoomReadOnly to the import list (alongside InvalidPayload, NotAuthenticated, RoomLocked, RoomNotFound, problem_responses) and include RoomReadOnly in the problem_responses(...) call used on the upload_trajectory handler (the decorator that now also uses RequireWritableDep) so the handler's Problem JSON response for read-only rooms is documented.
156-189:⚠️ Potential issue | 🟠 MajorValidate
selectionagainst every exported frame before streaming.Line 160 only checks the first selected frame. If a later frame has fewer atoms, Line 186 can still raise inside
_generate()after the 200 response has already started, which turns a bad request into a truncated download.Proposed fix
mime_type, ext = _FORMAT_INFO[format] + download_batch = 100 # Validate atom selection against the first frame (before streaming starts) if atom_indices is not None: - first_frame = await storage[room_id][index_list[0]] - if first_frame is not None: - n_atoms = len(decode(first_frame)) - invalid_atoms = [i for i in atom_indices if i < 0 or i >= n_atoms] - if invalid_atoms: - raise InvalidPayload.exception( - f"Atom indices {invalid_atoms} out of range (0-{n_atoms - 1})" - ) + for batch_start in range(0, len(index_list), download_batch): + raw_frames = await storage[room_id][ + index_list[batch_start : batch_start + download_batch] + ].to_list() + for raw_frame in raw_frames: + if raw_frame is None: + continue + n_atoms = len(decode(raw_frame)) + invalid_atoms = [i for i in atom_indices if i < 0 or i >= n_atoms] + if invalid_atoms: + raise InvalidPayload.exception( + f"Atom indices {invalid_atoms} out of range (0-{n_atoms - 1})" + ) @@ - download_batch = 100 - async def _generate():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/routes/trajectory.py` around lines 156 - 189, Currently only the first selected frame is validated which can let later frames trigger an out-of-range error during streaming; before yielding any data in _generate(), iterate all frames in index_list (using storage[room_id][...] and decode(...) as done now) to compute n_atoms per frame and collect any invalid atom indices relative to each frame, and if any invalids are found raise InvalidPayload.exception (same as existing) before starting the response so the request fails fast rather than producing a truncated download; keep references to atom_indices, index_list, storage, decode, and _generate when locating where to add this pre-check.tests/test_routes_frames.py (1)
151-335: 🛠️ Refactor suggestion | 🟠 MajorPlease parameterize repeated endpoint test matrices.
A large set of touched tests follows the same arrange/act/assert structure with only small input/expected variations. Converting these groups to
pytest.mark.parametrizewill reduce duplication and maintenance overhead. As per coding guidelines:Use pytest.mark.parametrize for tests. Each test must be a function, not a class method.Also applies to: 343-403, 494-752, 847-935, 965-1024
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_routes_frames.py` around lines 151 - 335, Many of the test functions (e.g., test_list_frames_empty_room, test_list_frames_with_data, test_list_frames_with_range, test_list_frames_with_indices, test_list_frames_with_keys_filter, test_list_frames_with_indices_and_keys) repeat the same arrange/act/assert pattern with only inputs and expected outputs varying; replace these repetitive test functions with pytest.mark.parametrize-driven tests by grouping common setup (user, token, room creation and frame_storage population) into a single test function per scenario type and drive different inputs (storage payload, query params like start/stop/indices/keys, and expected frame dicts/counts) via parametrize tuples; ensure each parametrized case is a top-level test function (not a class method), preserve helper calls (_create_user, _create_room, make_raw_frame, raw_frame_to_dict, decode_msgpack_response, _auth_header) inside the test, and convert the other similar ranges mentioned (lines noted in the review: 343-403, 494-752, 847-935, 965-1024) in the same manner to remove duplication and improve maintainability.
🧹 Nitpick comments (4)
tests/test_trajectory.py (1)
421-651: Please lock in the provider-backed upload behavior with a dedicated test.Lines 421-651 only exercise normal writable rooms. Since this PR moved the upload guard to
RequireWritableDep, add one case that doesawait frame_storage.set_frame_count(room.id, 1)and asserts the provider-backed room returns the read-only problem response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_trajectory.py` around lines 421 - 651, Add a test that exercises the provider-backed (read-only) upload guard: create user and room as in other tests, call await frame_storage.set_frame_count(room.id, 1) to mark the room provider-backed, then POST a trajectory file to f"/v1/rooms/{room.id}/trajectory" with authentication and assert the response.status_code is 423 and that ProblemDetail.model_validate(response.json()).type == ReadOnlyRoom.type_uri() (this verifies the RequireWritableDep guard triggers for provider-backed rooms); name the test something like test_upload_provider_backed_readonly and mirror the upload payload used elsewhere.tests/test_isosurface.py (1)
377-673: Add a provider-backed isosurface regression here.Lines 377-673 only cover frames that are already materialized in
frame_storage. After theFrameStorageswap, the storage-miss branch for mounted rooms is still untested, which is the path that can quietly regress back into a 404.Based on learnings: In ZnDraw, isosurface/volumetric cube data can be stored directly in frames or generated on demand by provider-backed rooms, so this route must keep the provider-dispatch/materialization flow instead of treating a miss as a hard 404.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_isosurface.py` around lines 377 - 673, Add a provider-backed isosurface regression test that exercises the storage-miss → provider materialization path: create a room configured as a mounted/provider-backed room (using the same helper used elsewhere, e.g., create_test_room with provider/mounted flags or the provider registration helpers), do NOT pre-populate frame_storage for that room, ensure the test registers a simple provider stub that returns cube data when the provider dispatch is invoked, then call the isosurface endpoint (as in test_isosurface_basic) for frame index 0 with the provider-only cube_key and assert a 200 plus non-empty "vertices" and "faces" in the msgpack response; this verifies the code paths around provider dispatch/materialize (the mounted-room branch) instead of treating a storage miss as a 404.tests/test_frames_provider_dispatch.py (1)
28-28:RawFrameimport now conflicts with how payloads are typed in this test file.On Line 28,
RawFrameis imported, but the local provider-cache payloads are dict-like maps (dict[bytes, bytes]), not theRawFramewrapper object. Tightening these annotations will avoid misleading type hints.💡 Suggested cleanup
-from zndraw.storage import FrameStorage, RawFrame +from zndraw.storage import FrameStorage# In the payload-building spots, prefer: frame_data: dict[bytes, bytes] = {b"arrays.positions": positions_packed}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_frames_provider_dispatch.py` at line 28, The test imports RawFrame from zndraw.storage but the provider-cache payloads in tests are plain dict[bytes, bytes], not RawFrame instances; update the type annotations and any variable declarations that currently use RawFrame to use dict[bytes, bytes] (or a more generic Mapping[bytes, bytes]) so payload-building sites (where frame_data is constructed) reflect the actual structure, and ensure references to FrameStorage remain unchanged if they expect wrapper objects; search for RawFrame usages in this file and replace the type with dict[bytes, bytes] (or Mapping) and adjust any docstrings or comments accordingly.tests/test_routes_frames.py (1)
32-32:raw_frame_to_dictis annotated asRawFramebut treated as a mapping.The helper calls
.items()and decodes key/value pairs like adict[bytes, bytes]. Updating the annotation/import will keep typing aligned with runtime behavior.💡 Suggested fix
-from zndraw.storage import FrameStorage, RawFrame +from zndraw.storage import FrameStorage @@ -def raw_frame_to_dict(frame: RawFrame) -> dict[str, Any]: +def raw_frame_to_dict(frame: dict[bytes, bytes]) -> dict[str, Any]:Also applies to: 46-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_routes_frames.py` at line 32, The helper raw_frame_to_dict is currently annotated/typed as RawFrame but the function treats it like a mapping (calling .items() and decoding key/value bytes); update the type annotation and any imports so raw_frame_to_dict accepts a Mapping[bytes, bytes] (or dict[bytes, bytes]) instead of RawFrame and adjust the import statements if needed (e.g., import Mapping from typing) so the annotation matches runtime behavior; also apply the same change for the other helper usages around lines where raw_frame_to_dict is used (the block referenced in the review).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_constraints.py`:
- Around line 138-140: The lmdb_storage fixture constructs FrameStorage with a
bare filesystem path which the backend selector rejects; update the lmdb_storage
fixture to pass a schemed URI (use the file:// scheme) and an absolute path when
creating FrameStorage (refer to the lmdb_storage fixture and the FrameStorage
constructor) so the storage backend selector receives a valid URI like
file://<absolute-path> instead of a plain path.
---
Outside diff comments:
In `@src/zndraw/routes/trajectory.py`:
- Around line 33-39: The endpoint's OpenAPI problem responses omit the new
read-only error branch: add RoomReadOnly to the import list (alongside
InvalidPayload, NotAuthenticated, RoomLocked, RoomNotFound, problem_responses)
and include RoomReadOnly in the problem_responses(...) call used on the
upload_trajectory handler (the decorator that now also uses RequireWritableDep)
so the handler's Problem JSON response for read-only rooms is documented.
- Around line 156-189: Currently only the first selected frame is validated
which can let later frames trigger an out-of-range error during streaming;
before yielding any data in _generate(), iterate all frames in index_list (using
storage[room_id][...] and decode(...) as done now) to compute n_atoms per frame
and collect any invalid atom indices relative to each frame, and if any invalids
are found raise InvalidPayload.exception (same as existing) before starting the
response so the request fails fast rather than producing a truncated download;
keep references to atom_indices, index_list, storage, decode, and _generate when
locating where to add this pre-check.
In `@tests/test_routes_frames.py`:
- Around line 151-335: Many of the test functions (e.g.,
test_list_frames_empty_room, test_list_frames_with_data,
test_list_frames_with_range, test_list_frames_with_indices,
test_list_frames_with_keys_filter, test_list_frames_with_indices_and_keys)
repeat the same arrange/act/assert pattern with only inputs and expected outputs
varying; replace these repetitive test functions with
pytest.mark.parametrize-driven tests by grouping common setup (user, token, room
creation and frame_storage population) into a single test function per scenario
type and drive different inputs (storage payload, query params like
start/stop/indices/keys, and expected frame dicts/counts) via parametrize
tuples; ensure each parametrized case is a top-level test function (not a class
method), preserve helper calls (_create_user, _create_room, make_raw_frame,
raw_frame_to_dict, decode_msgpack_response, _auth_header) inside the test, and
convert the other similar ranges mentioned (lines noted in the review: 343-403,
494-752, 847-935, 965-1024) in the same manner to remove duplication and improve
maintainability.
---
Nitpick comments:
In `@tests/test_frames_provider_dispatch.py`:
- Line 28: The test imports RawFrame from zndraw.storage but the provider-cache
payloads in tests are plain dict[bytes, bytes], not RawFrame instances; update
the type annotations and any variable declarations that currently use RawFrame
to use dict[bytes, bytes] (or a more generic Mapping[bytes, bytes]) so
payload-building sites (where frame_data is constructed) reflect the actual
structure, and ensure references to FrameStorage remain unchanged if they expect
wrapper objects; search for RawFrame usages in this file and replace the type
with dict[bytes, bytes] (or Mapping) and adjust any docstrings or comments
accordingly.
In `@tests/test_isosurface.py`:
- Around line 377-673: Add a provider-backed isosurface regression test that
exercises the storage-miss → provider materialization path: create a room
configured as a mounted/provider-backed room (using the same helper used
elsewhere, e.g., create_test_room with provider/mounted flags or the provider
registration helpers), do NOT pre-populate frame_storage for that room, ensure
the test registers a simple provider stub that returns cube data when the
provider dispatch is invoked, then call the isosurface endpoint (as in
test_isosurface_basic) for frame index 0 with the provider-only cube_key and
assert a 200 plus non-empty "vertices" and "faces" in the msgpack response; this
verifies the code paths around provider dispatch/materialize (the mounted-room
branch) instead of treating a storage miss as a 404.
In `@tests/test_routes_frames.py`:
- Line 32: The helper raw_frame_to_dict is currently annotated/typed as RawFrame
but the function treats it like a mapping (calling .items() and decoding
key/value bytes); update the type annotation and any imports so
raw_frame_to_dict accepts a Mapping[bytes, bytes] (or dict[bytes, bytes])
instead of RawFrame and adjust the import statements if needed (e.g., import
Mapping from typing) so the annotation matches runtime behavior; also apply the
same change for the other helper usages around lines where raw_frame_to_dict is
used (the block referenced in the review).
In `@tests/test_trajectory.py`:
- Around line 421-651: Add a test that exercises the provider-backed (read-only)
upload guard: create user and room as in other tests, call await
frame_storage.set_frame_count(room.id, 1) to mark the room provider-backed, then
POST a trajectory file to f"/v1/rooms/{room.id}/trajectory" with authentication
and assert the response.status_code is 423 and that
ProblemDetail.model_validate(response.json()).type == ReadOnlyRoom.type_uri()
(this verifies the RequireWritableDep guard triggers for provider-backed rooms);
name the test something like test_upload_provider_backed_readonly and mirror the
upload payload used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 504c787c-9708-4276-87d6-e6998412537d
📒 Files selected for processing (12)
src/zndraw/routes/trajectory.pytests/conftest.pytests/test_constraints.pytests/test_frame_storage.pytests/test_frames_provider_dispatch.pytests/test_isosurface.pytests/test_lifespan.pytests/test_result_backends.pytests/test_routes_frames.pytests/test_routes_step.pytests/test_storage_router.pytests/test_trajectory.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_frame_storage.py
Comment on lines
+138
to
+140
| async def lmdb_storage(tmp_path, redis_client): | ||
| """Create a fresh FrameStorage with LMDB backend.""" | ||
| storage = FrameStorage(str(tmp_path / "test.lmdb"), redis_client) |
There was a problem hiding this comment.
LMDB fixture passes an unsupported URI format to FrameStorage.
On Line 140, FrameStorage(str(tmp_path / "test.lmdb"), redis_client) uses a bare path. The storage backend selector expects a schemed URI (file://... / sqlite://... / redis://...), so this can raise ValueError once lmdb_storage[room_id] is used.
🔧 Suggested fix
- storage = FrameStorage(str(tmp_path / "test.lmdb"), redis_client)
+ storage = FrameStorage(f"file://{tmp_path / 'test.lmdb'}", redis_client)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_constraints.py` around lines 138 - 140, The lmdb_storage fixture
constructs FrameStorage with a bare filesystem path which the backend selector
rejects; update the lmdb_storage fixture to pass a schemed URI (use the file://
scheme) and an absolute path when creating FrameStorage (refer to the
lmdb_storage fixture and the FrameStorage constructor) so the storage backend
selector receives a valid URI like file://<absolute-path> instead of a plain
path.
Member
Author
There was a problem hiding this comment.
The asebytes registry handles bare LMDB paths — this is how LMDB has always been configured in this project. The test passes.
There was a problem hiding this comment.
@PythonFZ, thanks for the clarification! Understood — the asebytes registry accepts bare filesystem paths for LMDB backends, so no URI scheme is needed. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: PythonFZ
Repo: zincware/ZnDraw PR: 877
File: tests/test_constraints.py:138-140
Timestamp: 2026-03-19T10:02:26.990Z
Learning: In the ZnDraw project (zincware/ZnDraw), `FrameStorage` (and previously `AsebytesStorage`) can be initialized with bare LMDB filesystem paths (e.g., `str(tmp_path / "test.lmdb")`) without a URI scheme. The `asebytes` registry handles bare paths for LMDB backends — this is the established convention in this project. Do not flag bare LMDB paths as invalid URIs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: PythonFZ
Repo: zincware/ZnDraw PR: 877
File: src/zndraw/storage/frame_storage.py:20-31
Timestamp: 2026-03-19T10:02:10.740Z
Learning: In `src/zndraw/storage/frame_storage.py`, the `to_raw_frame` function unconditionally base64-decodes all `str` values. This is intentional: values arriving through the frontend JSON API path are always base64-encoded binary blobs (msgpack-numpy data). Keys use the `b64:` marker to distinguish plain string keys from encoded bytes keys, but values are always binary — no plain string values exist in the ZnDraw frame protocol. This behavior is preserved from the original `AsebytesStorage` implementation.
Add RoomReadOnly to problem_responses decorator so the OpenAPI contract documents the 409 response from RequireWritableDep. Add integration test verifying provider-backed rooms reject trajectory uploads. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/zndraw/routes/trajectory.py (2)
159-168:⚠️ Potential issue | 🟠 MajorValidate
selectionagainst every exported frame before streaming.This only checks the first requested frame. If a later frame is shorter,
atoms[atom_indices]can still fail after the response has started, so the client gets a truncated200instead of a cleanInvalidPayload. Please pre-validate the fullindex_listin batches before constructingStreamingResponse, and add a regression test with mixed atom counts.Also applies to: 186-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/routes/trajectory.py` around lines 159 - 168, The current validation in trajectory.py only checks atom_indices against the first frame and can miss later frames with fewer atoms; update the code that builds the streaming response to pre-validate every frame referenced by index_list (use storage[room_id][idx] and decode() in batches to avoid loading everything at once) and compute n_atoms per frame to collect any invalid_atoms across all frames, then raise InvalidPayload.exception if any are out of range before creating the StreamingResponse; apply the same full-frame validation logic to the other selection check around the StreamingResponse creation (the block referenced at lines ~186-187) and add a regression test that streams an index_list with mixed atom counts to ensure selection validation fails cleanly.
238-311:⚠️ Potential issue | 🟠 MajorHandle non-UTF8 uploads as a 400, not a 500.
content.decode("utf-8")runs before thetry, so invalid bytes bypassInvalidPayload.exception()and surface as an internal error.As per coding guidelines "Use RFC 9457 Problem JSON (application/problem+json) for all 4xx/5xx REST API responses".💡 Proposed fix
- buf = io.StringIO(content.decode("utf-8")) - try: - atoms_list = ase.io.read(buf, index=":", format=fmt) + try: + buf = io.StringIO(content.decode("utf-8")) + atoms_list = ase.io.read(buf, index=":", format=fmt) + except UnicodeDecodeError as exc: + raise InvalidPayload.exception( + "Trajectory file must be valid UTF-8 text" + ) from exc except Exception as exc: raise InvalidPayload.exception( f"Failed to parse trajectory file: {exc}" ) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/routes/trajectory.py` around lines 238 - 311, The handler upload_trajectory decodes the uploaded bytes with content.decode("utf-8") outside the try block so invalid UTF-8 raises a UnicodeDecodeError that becomes a 500; move the decoding into the existing try that calls ase.io.read (or add a small try around the decode) and catch UnicodeDecodeError (and other parsing errors) to raise InvalidPayload.exception with a clear 400-level message (e.g., "Uploaded file is not valid UTF-8" or "Failed to parse trajectory file: ..."); update references to buf (io.StringIO) so it is created after successful decoding and keep the ase.io.read(...) call inside the same error-handling block to ensure non-UTF8 uploads return a 400 not a 500.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/zndraw/routes/trajectory.py`:
- Around line 159-168: The current validation in trajectory.py only checks
atom_indices against the first frame and can miss later frames with fewer atoms;
update the code that builds the streaming response to pre-validate every frame
referenced by index_list (use storage[room_id][idx] and decode() in batches to
avoid loading everything at once) and compute n_atoms per frame to collect any
invalid_atoms across all frames, then raise InvalidPayload.exception if any are
out of range before creating the StreamingResponse; apply the same full-frame
validation logic to the other selection check around the StreamingResponse
creation (the block referenced at lines ~186-187) and add a regression test that
streams an index_list with mixed atom counts to ensure selection validation
fails cleanly.
- Around line 238-311: The handler upload_trajectory decodes the uploaded bytes
with content.decode("utf-8") outside the try block so invalid UTF-8 raises a
UnicodeDecodeError that becomes a 500; move the decoding into the existing try
that calls ase.io.read (or add a small try around the decode) and catch
UnicodeDecodeError (and other parsing errors) to raise InvalidPayload.exception
with a clear 400-level message (e.g., "Uploaded file is not valid UTF-8" or
"Failed to parse trajectory file: ..."); update references to buf (io.StringIO)
so it is created after successful decoding and keep the ase.io.read(...) call
inside the same error-handling block to ensure non-UTF8 uploads return a 400 not
a 500.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed83f563-e22c-47a1-9134-abe3f130dfa0
📒 Files selected for processing (2)
src/zndraw/routes/trajectory.pytests/test_trajectory.py
Non-UTF8 file uploads now return 422 InvalidPayload instead of 500. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AsebytesStorage+StorageRouter(two wrapper layers with 6_backendSLF001 violations) with a single thinFrameStorageregistry classio[index],io[start:stop].to_list(),io[b"key"][0], etc.) directlyRequireWritableDep)FrameStorageDepreplaces bothStorageDepandStorageRouterDep(fixes broken typing where runtime wasStorageRouterbut type saidAsebytesStorage)What's deleted
AsebytesStorageclass,StorageRouterclass,storage/router.pyStorageDep,StorageRouterDep,get_storage(),get_storage_router()reserve(),remove_items()(dead code — never called from routes)noqa: SLF001suppressions (zero_backendaccess)to_raw_frame()calls inside storage methodstry/except NotImplementedErrorinupdate_roomWhat's new
FrameStorage— room-scopedAsyncBlobIOregistry + provider frame count metadata (Redis). No read/write wrappers.RequireWritableDep— FastAPI dependency for write guard on mutation endpointsstorage[room_id]to getAsyncBlobIO, then subscript API for all operationsPerformance
E2E benchmarks (LMDB backend, 200-500 iterations per test, stdev-aware significance):
Zero regressions — all 16 benchmarks show "same" within noise. Payload reduction of 92.8% for filtered key reads (34KB → 2.5KB).
Test plan
SLF001violations in storage/routesAsebytesStorage,StorageRouter,StorageDep) anywhere🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests
Documentation