Conversation
for more information, see https://pre-commit.ci
WalkthroughConsolidated many function-local imports to module-level scope, standardized import ordering, removed the "settings" extension key from public extension lookup, changed settings-invalidation to clear the full settings cache, and adjusted PathTracing fields from float to int in settings.py. Several test imports and small formatting adjustments were also applied. Changes
Sequence Diagram(s)(no sequence diagram generated — changes are primarily import consolidation, minor behavioral shifts, and type adjustments without a new multi-component sequential feature) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
app/src/hooks/useLazyRoomData.ts (1)
35-35: UnusedqueryClientvariable.
queryClientis declared but never used in this hook. Consider removing it to reduce dead code.🔎 Proposed fix
export const useLazyRoomData = (roomId: string | null, enabled: boolean) => { - const queryClient = useQueryClient(); const { setFrameCount,Also update the import on line 2:
-import { useQuery, useQueryClient } from "@tanstack/react-query"; +import { useQuery } from "@tanstack/react-query";src/zndraw/app/utility_routes.py (1)
167-167: Consider consolidating duplicatecreate_jwt_tokenimport.The
from zndraw.auth import create_jwt_tokenis imported identically in bothlogin()(line 167) andregister_user()(line 331) functions. Since this PR is addressing lazy imports, consolidating this to a single module-level import would reduce duplication and align with the overall refactoring intent.🔎 Proposed consolidation
Add to module-level imports (after line 15):
from zndraw.auth import AuthError, get_current_user, require_admin, require_auth +from zndraw.auth import create_jwt_tokenThen remove lines 167 and 331 from within the functions.
Also applies to: 331-331
app/src/hooks/usePropertyInspectorSettings.ts (1)
44-52: Verify safe access to nested property.The code assumes
settingsResponse.data.property_inspector.enabled_propertiesexists once loaded. While there's a guard for loading state, consider adding optional chaining or a more defensive check to handle cases where the backend might not return the expected structure:const enabledProperties: string[] = - isLoading || !settingsResponse - ? [] - : settingsResponse.data.property_inspector.enabled_properties; + isLoading || !settingsResponse || !settingsResponse.data.property_inspector + ? [] + : settingsResponse.data.property_inspector.enabled_properties ?? [];This ensures the code won't throw if the nested path is unexpectedly undefined.
Can you verify that the backend always returns
data.property_inspector.enabled_propertiesin the settings response? If not guaranteed, the suggested defensive check would prevent runtime errors.tests/test_vis_settings.py (1)
9-44: Test structure follows guidelines - consider adding request timeouts.The test correctly validates the consolidated settings endpoint response structure and defaults. Each test function is appropriately focused on a single concern.
However, the static analysis tool flagged that
requests.getandrequests.putcalls lack timeouts (S113). While less critical in tests, adding timeouts improves CI reliability by preventing indefinite hangs.🔎 Example timeout addition
- response = requests.get(f"{server}/api/rooms/{room_id}/settings", headers=headers) + response = requests.get(f"{server}/api/rooms/{room_id}/settings", headers=headers, timeout=10)Apply similar changes to all
requests.getandrequests.putcalls in this file.src/zndraw/app/worker_routes.py (1)
153-165: Minor: Variable shadowing in set comprehension.The loop variable
namein the set comprehension shadows the function parameternamefrom line 97. While this doesn't cause a bug (the comprehension creates its own scope), it could cause confusion during debugging.🔎 Suggested rename for clarity
# Settings category names from RoomConfig (excludes inherited fields like callback) settings_names = { - name - for name, field in RoomConfig.model_fields.items() + field_name + for field_name, field in RoomConfig.model_fields.items() if field.default_factory is not None }src/zndraw/app/settings_routes.py (2)
77-86: Consider reducing log verbosity for user data.Line 85 logs the full
json_datapayload at INFO level, which may include user-specific settings data. Consider logging at DEBUG level or limiting to category keys only for production safety.🔎 Proposed fix
settings_service = current_app.extensions["settings_service"] - log.info(f"update_settings received data: {json_data}") + log.debug(f"update_settings received data for categories: {list(json_data.keys())}") settings_service.update_all(room_id, user_name, json_data)
99-104: Logging user identifiers at INFO level.Logging
user_nameat INFO level could raise compliance/privacy concerns depending on whether usernames are considered PII. Consider using DEBUG level or anonymizing user references in production logs.src/zndraw/services/settings_service.py (1)
70-85: Consider validating incoming categories before storing.The
update_allmethod stores whatever categories are passed without validation. While the route layer validates againstRoomConfig.model_fields, if this service is called from elsewhere (e.g., internal code), invalid categories could be stored. Consider adding validation here for defense in depth, or document that callers must pre-validate.src/zndraw/settings.py (1)
143-172: Useintfor discrete rendering parameters.Fields like
min_samples,samples,bounces, andtilesrepresent discrete rendering counts but are typed asfloat. This allows fractional values (e.g., 2.5 bounces) which are invalid for a path tracer. The frontend passes these directly to the Pathtracer component which expects integers, and the TypeScript types are auto-generated asnumberanyway. Changing tointwould prevent invalid input and better reflect the semantic purpose of these fields.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
app/src/components/Canvas.tsxapp/src/components/SettingsPanel.tsxapp/src/hooks/useLazyRoomData.tsapp/src/hooks/usePropertyInspectorSettings.tsapp/src/hooks/useSettings.tsapp/src/hooks/useSocketManager.tsapp/src/myapi/client.tssrc/zndraw/api_manager.pysrc/zndraw/app/extension_routes.pysrc/zndraw/app/file_browser.pysrc/zndraw/app/geometry_routes.pysrc/zndraw/app/room_routes.pysrc/zndraw/app/settings_routes.pysrc/zndraw/app/utility_routes.pysrc/zndraw/app/worker_routes.pysrc/zndraw/cli.pysrc/zndraw/job_executor.pysrc/zndraw/services/settings_service.pysrc/zndraw/services/user_service.pysrc/zndraw/settings.pysrc/zndraw/socket_manager.pysrc/zndraw/utils.pysrc/zndraw/zndraw.pytests/test_lazy_room_loading.pytests/test_vis_settings.py
💤 Files with no reviewable changes (1)
- src/zndraw/utils.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Docstrings must be concise and to the point
Use type hints wherever possible. Importtyping as tif necessary, but uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
src/zndraw/app/utility_routes.pysrc/zndraw/cli.pysrc/zndraw/app/room_routes.pysrc/zndraw/app/geometry_routes.pytests/test_lazy_room_loading.pysrc/zndraw/job_executor.pysrc/zndraw/api_manager.pysrc/zndraw/app/settings_routes.pytests/test_vis_settings.pysrc/zndraw/services/user_service.pysrc/zndraw/app/file_browser.pysrc/zndraw/app/worker_routes.pysrc/zndraw/zndraw.pysrc/zndraw/services/settings_service.pysrc/zndraw/app/extension_routes.pysrc/zndraw/settings.pysrc/zndraw/socket_manager.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test_*.py: Usepytest.mark.parametrizeto avoid code duplication in tests
Tests should be very specific and test only one thing
Avoid complex test setups
Each test must be a function, not a method of a class
Files:
tests/test_lazy_room_loading.pytests/test_vis_settings.py
🧬 Code graph analysis (17)
app/src/components/Canvas.tsx (1)
app/src/hooks/useSettings.ts (1)
useSettings(22-33)
src/zndraw/app/room_routes.py (2)
src/zndraw/zndraw.py (1)
selections(657-673)src/zndraw/app/redis_keys.py (1)
selections(312-314)
src/zndraw/app/geometry_routes.py (4)
src/zndraw/zndraw.py (1)
geometries(630-631)src/zndraw/app/redis_keys.py (1)
geometries(304-306)src/zndraw/extensions/modifiers.py (1)
model_json_schema(344-353)src/zndraw/frame_cache.py (1)
items(42-43)
tests/test_lazy_room_loading.py (1)
tests/conftest.py (1)
server(263-269)
src/zndraw/job_executor.py (2)
src/zndraw/zndraw.py (1)
selections(657-673)src/zndraw/app/redis_keys.py (1)
selections(312-314)
src/zndraw/api_manager.py (1)
src/zndraw/app/settings_routes.py (2)
get_settings(24-46)update_settings(51-104)
src/zndraw/app/settings_routes.py (4)
src/zndraw/auth.py (2)
get_current_user(126-144)require_auth(168-188)src/zndraw/settings.py (1)
RoomConfig(200-208)src/zndraw/api_manager.py (2)
get_settings(554-569)update_settings(571-586)src/zndraw/services/settings_service.py (2)
get_all(32-68)update_all(70-85)
tests/test_vis_settings.py (1)
src/zndraw/settings.py (1)
RoomConfig(200-208)
app/src/hooks/usePropertyInspectorSettings.ts (1)
app/src/hooks/useSettings.ts (1)
useSettings(22-33)
app/src/hooks/useSettings.ts (1)
app/src/myapi/client.ts (3)
getSettings(570-575)updateSettings(577-586)SettingsResponse(565-568)
app/src/myapi/client.ts (1)
app/src/hooks/useSettings.ts (1)
SettingsResponse(16-16)
src/zndraw/app/worker_routes.py (2)
src/zndraw/zndraw.py (2)
selections(657-673)settings(1534-1554)src/zndraw/app/redis_keys.py (2)
selections(312-314)settings(324-337)
src/zndraw/zndraw.py (3)
src/zndraw/settings.py (1)
RoomConfig(200-208)src/zndraw/api_manager.py (2)
update_settings(571-586)get_settings(554-569)src/zndraw/app/settings_routes.py (2)
update_settings(51-104)get_settings(24-46)
src/zndraw/services/settings_service.py (2)
src/zndraw/zndraw.py (6)
settings(1534-1554)get(197-210)get(955-955)get(957-959)get(961-963)get(992-1085)src/zndraw/settings.py (1)
RoomConfig(200-208)
src/zndraw/app/extension_routes.py (3)
src/zndraw/auth.py (2)
get_current_user(126-144)require_auth(168-188)src/zndraw/zndraw.py (1)
selections(657-673)src/zndraw/app/redis_keys.py (1)
selections(312-314)
src/zndraw/socket_manager.py (2)
src/zndraw/bookmarks_manager.py (1)
clear(197-215)src/zndraw/frame_cache.py (1)
clear(35-36)
app/src/components/SettingsPanel.tsx (3)
app/src/formStore.ts (1)
useFormStore(15-34)app/src/hooks/useSettings.ts (2)
useSettings(22-33)useUpdateSettings(41-76)app/src/utils/jsonforms.ts (1)
injectDynamicEnums(91-140)
🪛 Ruff (0.14.10)
tests/test_vis_settings.py
16-16: Probable use of requests call without timeout
(S113)
52-52: Probable use of requests call without timeout
(S113)
59-59: Probable use of requests call without timeout
(S113)
67-67: Probable use of requests call without timeout
(S113)
84-84: Probable use of requests call without timeout
(S113)
95-95: Probable use of requests call without timeout
(S113)
110-110: Probable use of requests call without timeout
(S113)
🔇 Additional comments (38)
app/src/hooks/useLazyRoomData.ts (2)
113-115: LGTM!The documentation correctly reflects the architectural change where settings are now fetched on-demand via the consolidated
useSettingshook rather than being part of the lazy room data loading. This aligns with the batch API consolidation.
198-207: LGTM!The inline comment on line 199 appropriately documents that settings loading is excluded from the aggregate
isLoadingstate since they're fetched separately on-demand.src/zndraw/app/utility_routes.py (1)
6-9: Module-level import organization aligns with PR objective and coding guidelines.Moving
secretsfrom lazy import (inside thelogin()function) to module-level scope is correct and follows the guideline that "Imports should always be at the top of the file." Import ordering is also correct (stdlib → third-party → local).src/zndraw/app/file_browser.py (1)
5-6: LGTM! Import consolidation follows coding guidelines.Moving
reanduuidimports to module scope aligns with the project's coding guidelines that state "Imports should always be at the top of the file." This eliminates redundant per-function imports without changing behavior.As per coding guidelines, imports should always be at the top of the file.
app/src/hooks/useSocketManager.ts (1)
149-153: LGTM! Cache invalidation updated for consolidated settings API.The invalidation key now correctly targets the consolidated settings cache at
["settings", roomId, userName], which aligns with theuseSettingshook's queryKey structure and the new batch settings endpoint.src/zndraw/socket_manager.py (2)
2-2: LGTM! Import consolidation follows coding guidelines.Moving
import reto module scope (used in_on_filesystem_listat line 357) aligns with the project's coding guidelines.As per coding guidelines, imports should always be at the top of the file.
290-292: LGTM! Settings invalidation updated for consolidated API.Clearing the entire settings cache instead of popping individual extensions aligns with the consolidated settings API where all settings are fetched and updated as a batch. This ensures consistency with the new
get_settings()/update_settings()endpoints.src/zndraw/api_manager.py (1)
554-586: LGTM! Settings API consolidated to batch endpoint.The refactored methods correctly implement the consolidated settings approach:
get_settings()fetches all categories in one request, returning{"schema": ..., "data": ...}update_settings(data)accepts a dict keyed by category for batch updates- Added
timeout=10to both requests for better reliabilityThe API surface changes are breaking but align with the PR's goal of consolidating per-category endpoints into a unified batch API.
src/zndraw/services/user_service.py (1)
7-7: LGTM! Import consolidation follows coding guidelines.Moving
import datetimeto module scope eliminates redundant per-function imports and aligns with the project's coding guidelines.As per coding guidelines, imports should always be at the top of the file.
src/zndraw/cli.py (1)
2-2: LGTM! Import consolidation follows coding guidelines.Moving
import secretsto module scope (used for shutdown token generation at line 517) eliminates the inner import and aligns with the project's coding guidelines.As per coding guidelines, imports should always be at the top of the file.
tests/test_lazy_room_loading.py (1)
21-21: LGTM! Test improvements for reliability and API consolidation.The changes add
timeout=10to all HTTP requests, improving test reliability by preventing hangs. The settings endpoint assertions correctly validate the new consolidated response structure with"schema"and"data"keys.Also applies to: 32-32, 40-42, 51-51, 59-60, 68-69, 75-77, 84-94
src/zndraw/app/room_routes.py (1)
6-28: LGTM! Imports moved to module scope as per coding guidelines.The imports for
datetime,re,uuid, and the extension modules (analysis,modifiers,selections) have been correctly relocated to the top of the file. This aligns with the coding guideline: "Imports should always be at the top of the file."Note: The
selectionsimport fromzndraw.extensions.selectionsdoesn't conflict withRoomKeys.selections()since they exist in different namespaces.src/zndraw/app/extension_routes.py (1)
9-16: LGTM! Imports consolidated at module scope.The
datetimeclass import and extension module imports have been correctly moved to the top of the file, removing the need for in-function imports. This follows the coding guideline for placing imports at the top of the file.src/zndraw/job_executor.py (2)
13-15: LGTM! Extension imports moved to module scope.The imports for
analysis,modifiers, andselectionshave been correctly relocated to the top of the file, consistent with the coding guidelines.
227-231: Settings category intentionally removed from extension lookup.The
category_mapno longer includes"settings"since settings are now handled by dedicated/api/rooms/{roomId}/settingsendpoints rather than as an extension category. This aligns with the PR's consolidation of settings management.tests/test_vis_settings.py (1)
46-116: LGTM! Comprehensive tests for consolidated settings API.The tests appropriately cover:
- Partial updates (single category)
- Multi-category batch updates
- Error handling for invalid categories
Each test follows the guideline of testing one specific behavior.
src/zndraw/app/worker_routes.py (1)
8-17: LGTM! Imports correctly moved to module scope.The imports for
datetime, extension modules, andRoomConfigare properly placed at the top of the file per coding guidelines.app/src/components/Canvas.tsx (3)
6-6: LGTM! Import updated for consolidated settings hook.The import correctly changes from
useSettingDatatouseSettingsto align with the new batch settings API.
65-68: LGTM! Single consolidated settings fetch.Using
useSettings(roomId)to fetch all settings in one call is more efficient than multiple per-category fetches. The hook correctly returns both loading state and response data.
87-109: LGTM! Proper loading guard and settings extraction.The loading guard correctly blocks rendering until settings are available, preventing potential undefined access errors. The comment on line 105 accurately notes that the backend always returns defaults, making the direct property access safe.
src/zndraw/zndraw.py (3)
19-27: LGTM! Extension and settings imports at module scope.The imports for
analysis,modifiers,selections, andRoomConfigare correctly placed at the top of the file per coding guidelines.
46-67: LGTM! Celery extension check now includes analysis category.The
_is_celery_extensionfunction correctly uses the module-levelcategory_mapthat now includes"analysis"alongside"modifiers"and"selections".
1533-1554: LGTM! Settings property refactored for batch API.The implementation correctly:
- Defines a callback to push individual category updates via
update_settings({category: data})- Uses lazy initialization - only fetches on first access when
_settingsis empty- Dynamically iterates
RoomConfig.model_fieldsto populate categories- Uses
functools.partialto bind the category name to each callbackThe guard
if field_info.default_factory is Nonecorrectly skips non-settings fields.app/src/myapi/client.ts (2)
565-586: LGTM! Settings API surface updated for consolidated endpoint.The changes correctly:
- Rename
SettingsSchematoSettingsResponsewithschemaanddatafields- Rename
getSettingsSchemastogetSettingstargeting/api/rooms/{roomId}/settings- Refactor
updateSettingsto acceptsettingsData: Record<string, any>for bulk updatesThis aligns with the server-side consolidation of settings endpoints.
709-712: LGTM! Blob construction improved for TypeScript compatibility.Creating a new
Uint8Arrayfrom the encoded data ensures theBlobParttype is satisfied and prevents potential issues with underlying ArrayBuffer boundaries.app/src/hooks/useSettings.ts (2)
22-33: LGTM!The consolidated
useSettingshook is well-structured. The query is properly disabled whenroomIdoruserNameis missing, and usingstaleTime: Infinitywith socket-based invalidation is a sensible caching strategy.
41-76: LGTM!The mutation hook correctly implements optimistic updates by merging submitted data into the cached settings. The cache key alignment with the query ensures consistency. Error handling logs appropriately.
src/zndraw/app/settings_routes.py (1)
22-46: LGTM!The consolidated GET endpoint correctly returns both schema and data in a single response. The authentication via
@require_authand service delegation are properly implemented.app/src/components/SettingsPanel.tsx (4)
41-50: LGTM!The consolidated hook usage is clean. Passing
roomId ?? ""is safe sinceuseSettingshas anenabledguard that prevents fetching whenroomIdis falsy. The mutation hook is correctly destructured.
63-77: LGTM!The
$refresolution logic correctly handles JSON Schema references by extracting the ref name and looking it up in$defs. Including$defsin the resolved schema ensures nested references work properly.
138-149: LGTM!Early returns are correctly placed after all hooks are called, complying with React's Rules of Hooks. The loading and error states are handled appropriately.
185-198: LGTM!The conditional rendering ensures
JsonFormsonly renders when both a category is selected and the dynamic schema is available. Thekeyprop onJsonFormsensures proper re-mounting when the category changes.src/zndraw/services/settings_service.py (2)
29-31: LGTM!Clean helper method for constructing the Redis key with consistent naming convention.
47-68: LGTM!The implementation correctly handles Redis's potential to return either bytes or strings depending on the
decode_responsessetting. The fallback todefault_factoryfor missing categories ensures clients always receive complete data with defaults applied.src/zndraw/settings.py (2)
55-101: LGTM!The
json_schema_extraannotations provide rich UI hints (color picker, range sliders with step values) that will enhance the frontend form rendering. The field constraints (ge,le) are well-defined.
200-208: LGTM!Using
default_factoryfor nested settings sections is the correct pattern with Pydantic to avoid mutable default pitfalls. Themodel_configenables assignment validation and population by field name for flexibility.src/zndraw/app/geometry_routes.py (2)
56-56: LGTM! All usages consistently updated.All references to the imported
geometrieshave been correctly updated to use thegeometry_classesalias. The usage pattern (membership checks, dictionary access, and iteration via.items()) confirms the import is a mapping of geometry type names to their Pydantic model classes.Also applies to: 79-79, 189-191
11-11: Module-level import improves code organization and follows coding guidelines.The import has been correctly moved to module scope with a descriptive alias that avoids the naming conflict with the Blueprint on line 21. The
geometriesobject is a dict mapping geometry class names to their implementations, which already satisfies the collections.abc.Mapping interface requirement. This change aligns with the coding guideline requiring imports at the top of the file.
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 (6)
tests/test_vis_geometries.py (1)
535-535: Duplicatenumpyimport should be removed.Now that
numpyis imported at module level (line 1), this local import is redundant and should be removed. As per coding guidelines, imports should always be at the top of the file.🔎 Proposed fix
def test_color_already_exists(): """Test that update_colors_and_radii doesn't overwrite existing colors.""" import ase - import numpy as np from zndraw.utils import update_colors_and_radiitests/test_transformations.py (1)
19-20: Move imports to module-level scope.The
msgpackandmsgpack_numpyimports inside_decode_msgpack_dictviolate the coding guideline that "Imports should always be at the top of the file" and are inconsistent with this PR's objective to remove lazy route imports.🔎 Proposed fix
Move these imports to the top of the file with the other imports:
import ase import ase.constraints +import msgpack +import msgpack_numpy as m import pytest from asebytes import encode from zndraw.geometries import SphereAnd remove them from the function:
def _decode_msgpack_dict(obj): """Recursively decode msgpack dict[bytes, bytes] to regular Python dict.""" - import msgpack - import msgpack_numpy as m - if isinstance(obj, dict):As per coding guidelines, imports should always be at the top of the file.
src/zndraw-mcp/__init__.py (1)
26-26: Move function-local imports to module level.Lines 26 and 43 both import
from zndraw.geometries import geometriesinside function bodies. These should be consolidated at the top of the file per the coding guideline "Imports should always be at the top of the file" and the PR objective. No circular dependency exists—zndraw.geometriesdoes not depend onzndraw-mcp.Also applies to: 43-43
tests/test_prometheus_integration.py (2)
54-70: Test can pass vacuously if metric is not found.The assertion at line 69 only executes if
initial_value is not None. If theconnected_usersmetric isn't found in the registry (lines 55-59),initial_valueremainsNone, and the test passes without verifying the increment behavior.🔎 Proposed fix to ensure metric is found
initial_value = None for metric in REGISTRY.collect(): if metric.name == "connected_users": for sample in metric.samples: initial_value = sample.value break + + assert initial_value is not None, "connected_users metric not found in registry" # Increment connected_users.inc() # Check new value for metric in REGISTRY.collect(): if metric.name == "connected_users": for sample in metric.samples: - if initial_value is not None: - assert sample.value == initial_value + 1 + assert sample.value == initial_value + 1 break
83-99: Test can pass vacuously if metric is not found.The assertion at line 98 only executes if
current_value is not None. If theconnected_usersmetric isn't found in the registry (lines 84-88),current_valueremainsNone, and the test passes without verifying the decrement behavior.🔎 Proposed fix to ensure metric is found
current_value = None for metric in REGISTRY.collect(): if metric.name == "connected_users": for sample in metric.samples: current_value = sample.value break + + assert current_value is not None, "connected_users metric not found in registry" # Decrement connected_users.dec() # Check new value for metric in REGISTRY.collect(): if metric.name == "connected_users": for sample in metric.samples: - if current_value is not None: - assert sample.value == current_value - 1 + assert sample.value == current_value - 1 breaksrc/zndraw/app/worker_routes.py (1)
206-206: Replace deprecateddatetime.utcnow()withdatetime.now(timezone.utc).
datetime.utcnow()was deprecated in Python 3.12 and your project supports Python 3.12+ (requires-python = ">=3.11"). Replace withdatetime.now(timezone.utc)and update the import to includetimezonefrom the datetime module.Applies to lines 206 and 256.
🧹 Nitpick comments (7)
tests/test_transformations.py (1)
269-283: Consider usingpytest.mark.parametrizeto avoid the loop.The loop through different path formats could be replaced with
pytest.mark.parametrizefor better test isolation and clearer failure reporting.🔎 Proposed refactor
+@pytest.mark.parametrize( + "path", + [ + "0.kwargs.indices", # Array index + object keys + "FixAtoms.kwargs.indices", # Object keys only + "0.indices", # Shorter path + ], +) -def test_transform_with_nested_path(): +def test_transform_with_nested_path(path): """Test transform with deeply nested path.""" - # Test various path formats - paths = [ - "0.kwargs.indices", # Array index + object keys - "FixAtoms.kwargs.indices", # Object keys only - "0.indices", # Shorter path - ] - - for path in paths: - transform = InArrayTransform( - source="constraints", path=path, filter="arrays.positions" - ) - assert transform.path == path + transform = InArrayTransform( + source="constraints", path=path, filter="arrays.positions" + ) + assert transform.path == pathAs per coding guidelines, use
pytest.mark.parametrizeto avoid code duplication in tests.tests/test_prometheus_integration.py (4)
20-28: Consider moving imports to module level.Per the coding guidelines, imports should be at the top of the file. Since these configuration imports have no side effects, moving them to module level would improve consistency with the PR's objective of removing lazy imports and reduce import overhead.
🔎 Suggested refactor
Move the imports from lines 22-23 to the top of the file alongside other module-level imports.
As per coding guidelines, imports should always be at the top of the file.
31-44: Consider moving imports to module level.Per the coding guidelines, imports should be at the top of the file. Moving these configuration imports to module level would align with the PR's objective and improve consistency.
As per coding guidelines, imports should always be at the top of the file.
47-53: Import reordering looks correct; consider module-level placement.The reordering to import
REGISTRYbeforeconnected_usersmakes sense if metric registration has dependencies. However, per coding guidelines, consider moving all imports to module level to align with the PR's objective of removing lazy imports.As per coding guidelines, imports should always be at the top of the file.
73-82: Consider moving imports to module level.Per the coding guidelines, imports should be at the top of the file. Moving these imports to module level would align with the PR's objective of removing lazy imports and improve consistency.
As per coding guidelines, imports should always be at the top of the file.
src/zndraw/app/worker_routes.py (2)
73-73: Consider hoisting the duplicated auth import to module level.The
from zndraw.auth import AuthError, get_current_userimport appears in both functions (lines 73, 343). Per the coding guidelines, "Imports should always be at the top of the file." Moving this to module level would eliminate the duplication, reduce per-call overhead, and align with the PR's objective to remove lazy route imports.🔎 Proposed fix
Add at the top of the file after line 10:
from flask import Blueprint, current_app, request +from zndraw.auth import AuthError, get_current_user from zndraw.app.constants import SocketEventsThen remove the local imports at lines 73 and 343.
Also applies to: 343-343
221-221: Consider hoisting the duplicated job_dispatcher import to module level.The
from .job_dispatcher import assign_pending_jobs_for_extensionimport appears twice within the same function (lines 221, 284). Per the coding guidelines, "Imports should always be at the top of the file." Moving this to module level would eliminate the duplication and reduce import overhead.🔎 Proposed fix
Add at the top of the file after line 16:
from zndraw.extensions.selections import selections +from zndraw.app.job_dispatcher import assign_pending_jobs_for_extension from zndraw.server import socketioThen remove the local imports at lines 221 and 284.
Also applies to: 284-284
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
misc/conftest.pymisc/test_screenshots.pysrc/zndraw-mcp/__init__.pysrc/zndraw/app/frame_routes.pysrc/zndraw/app/geometry_routes.pysrc/zndraw/app/job_routes.pysrc/zndraw/app/room_routes.pysrc/zndraw/app/worker_routes.pysrc/zndraw/extensions/__init__.pysrc/zndraw/extensions/analysis.pysrc/zndraw/extensions/modifiers.pysrc/zndraw/extensions/selections.pysrc/zndraw/geometries/__init__.pysrc/zndraw/lock.pysrc/zndraw/server_manager.pysrc/zndraw/services/room_service.pysrc/zndraw/start_celery.pysrc/zndraw/zndraw.pytests/test_chunked_upload.pytests/test_chunked_upload_exceptions.pytests/test_connectivity_threshold.pytests/test_constraints.pytests/test_dual_namespace_extensions.pytests/test_extension_endpoint_separation.pytests/test_filesystem_endpoint_separation.pytests/test_filesystem_registration.pytests/test_job_errors.pytests/test_msgpack_encoding.pytests/test_prometheus_integration.pytests/test_public_extensions.pytests/test_room_listing_performance.pytests/test_schema_returns_both_namespaces.pytests/test_step_endpoints.pytests/test_transformations.pytests/test_vis_geometries.pytests/test_worker_registration.pytests/test_workers.py
💤 Files with no reviewable changes (2)
- src/zndraw/start_celery.py
- src/zndraw/geometries/init.py
✅ Files skipped from review due to trivial changes (17)
- src/zndraw/extensions/modifiers.py
- tests/test_worker_registration.py
- src/zndraw/extensions/analysis.py
- src/zndraw/services/room_service.py
- src/zndraw/app/job_routes.py
- tests/test_step_endpoints.py
- tests/test_public_extensions.py
- src/zndraw/extensions/init.py
- tests/test_dual_namespace_extensions.py
- src/zndraw/extensions/selections.py
- misc/test_screenshots.py
- tests/test_extension_endpoint_separation.py
- tests/test_workers.py
- tests/test_filesystem_endpoint_separation.py
- misc/conftest.py
- tests/test_constraints.py
- tests/test_room_listing_performance.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/zndraw/zndraw.py
- src/zndraw/app/geometry_routes.py
- src/zndraw/app/room_routes.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Docstrings must be concise and to the point
Use type hints wherever possible. Importtyping as tif necessary, but uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
src/zndraw/app/worker_routes.pytests/test_msgpack_encoding.pytests/test_prometheus_integration.pysrc/zndraw/server_manager.pytests/test_filesystem_registration.pytests/test_job_errors.pytests/test_schema_returns_both_namespaces.pytests/test_chunked_upload.pysrc/zndraw/app/frame_routes.pysrc/zndraw/lock.pytests/test_chunked_upload_exceptions.pysrc/zndraw-mcp/__init__.pytests/test_vis_geometries.pytests/test_connectivity_threshold.pytests/test_transformations.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test_*.py: Usepytest.mark.parametrizeto avoid code duplication in tests
Tests should be very specific and test only one thing
Avoid complex test setups
Each test must be a function, not a method of a class
Files:
tests/test_msgpack_encoding.pytests/test_prometheus_integration.pytests/test_filesystem_registration.pytests/test_job_errors.pytests/test_schema_returns_both_namespaces.pytests/test_chunked_upload.pytests/test_chunked_upload_exceptions.pytests/test_vis_geometries.pytests/test_connectivity_threshold.pytests/test_transformations.py
🧠 Learnings (1)
📚 Learning: 2025-12-16T15:11:16.642Z
Learnt from: CR
Repo: zincware/ZnDraw PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T15:11:16.642Z
Learning: Applies to **/*.py : Imports should always be at the top of the file
Applied to files:
tests/test_filesystem_registration.py
🧬 Code graph analysis (5)
src/zndraw/app/worker_routes.py (2)
src/zndraw/app/constants.py (1)
SocketEvents(4-25)src/zndraw/app/redis_keys.py (4)
ExtensionKeys(12-145)FilesystemKeys(149-238)SessionKeys(568-633)selections(312-314)
tests/test_job_errors.py (1)
src/zndraw/extensions/abc.py (2)
Category(10-13)Extension(16-31)
tests/test_schema_returns_both_namespaces.py (1)
src/zndraw/extensions/abc.py (2)
Category(10-13)Extension(16-31)
tests/test_chunked_upload.py (1)
src/zndraw/zndraw.py (4)
ZnDraw(365-1816)LocalSettings(265-293)atoms(784-786)atoms(789-791)
src/zndraw-mcp/__init__.py (2)
src/zndraw/zndraw.py (1)
ZnDraw(365-1816)src/zndraw/server_manager.py (2)
ServerInfo(16-44)get_server_status(195-223)
⏰ 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 (13)
tests/test_job_errors.py (1)
10-10: LGTM! Import reordering improves consistency.The alphabetical ordering of imports (Category before Extension) aligns with the PR's objective to reorganize imports for consistency across the codebase.
tests/test_schema_returns_both_namespaces.py (1)
11-13: LGTM! Import organization follows best practices.The added blank line properly separates standard library imports from project imports, and the alphabetical ordering of
Category, Extensionimproves consistency across the codebase.src/zndraw/lock.py (1)
1-5: LGTM! Import reorganization aligns with best practices.The consolidation of
threadingandwarningsimports to module-level scope reduces per-call import overhead and follows the coding guideline requiring imports at the top of the file. Both imports are properly utilized throughout the class (threadingfor Thread/Event,warningsfor warn calls at lines 177 and 211).tests/test_filesystem_registration.py (1)
1-7: LGTM! Import reorganization aligns with coding guidelines.The consolidation of imports to module-level scope follows the coding guidelines and PR objectives perfectly. Moving
timeandZnDrawimports to the top eliminates per-function import overhead and improves code organization.Based on learnings, imports should always be at the top of the file.
tests/test_vis_geometries.py (1)
1-15: LGTM!The import changes look good:
numpymoved to module level per coding guidelinesBoxcorrectly added and used in the parametrized test on line 416- Imports are now alphabetically ordered within the multi-line import block
src/zndraw/server_manager.py (1)
4-4: LGTM! Import correctly placed at module level.The logging import is now at the top of the file, which aligns with the coding guidelines and PR objectives to consolidate imports and reduce per-call overhead.
tests/test_transformations.py (1)
11-12: LGTM! Import consolidation aligns with PR objectives.Moving the
encodeimport to module-level scope is consistent with the coding guideline that imports should always be at the top of the file, and aligns with the PR's objective to remove lazy route imports.src/zndraw-mcp/__init__.py (1)
3-3: LGTM! Import consolidation aligns with PR objectives.The module-level imports correctly reduce per-call overhead:
subprocess(used inuploadfunction)numpy,pandas,plotly(used inrun_zndraw_scriptsandbox)Import ordering follows PEP 8 conventions (stdlib → third-party → local).
Also applies to: 7-10, 13-14
tests/test_connectivity_threshold.py (1)
1-1: LGTM!The import consolidation aligns with the PR objectives and coding guidelines.
tests/test_chunked_upload_exceptions.py (1)
3-4: LGTM!Moving
Mockandpatchto module level is correct.tests/test_msgpack_encoding.py (1)
11-11: LGTM!Moving
asebytesto module-level imports aligns with the PR objectives and coding guidelines.tests/test_prometheus_integration.py (1)
10-17: LGTM - Function-local import is appropriate here.Since this test specifically validates that the analytics module can be imported, the function-local import is the correct approach.
src/zndraw/app/worker_routes.py (1)
8-8: LGTM! Imports correctly hoisted to module level.These imports have been properly moved from function-local scope to module-level, reducing per-call import overhead and aligning with the coding guideline that "Imports should always be at the top of the file."
Also applies to: 12-16
| from asebytes import decode as asebytes_decode | ||
| from asebytes import encode as asebytes_encode |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file size and read the relevant sections
wc -l src/zndraw/app/frame_routes.pyRepository: zincware/ZnDraw
Length of output: 94
🏁 Script executed:
# Read lines around 1014-1015 to see the actual code and function context
sed -n '1005,1025p' src/zndraw/app/frame_routes.pyRepository: zincware/ZnDraw
Length of output: 958
🏁 Script executed:
# Also check the function definition to understand scope
sed -n '911,930p' src/zndraw/app/frame_routes.pyRepository: zincware/ZnDraw
Length of output: 789
🏁 Script executed:
# Search for all asebytes imports in the file
rg -n 'from asebytes import' src/zndraw/app/frame_routes.pyRepository: zincware/ZnDraw
Length of output: 261
🏁 Script executed:
# Check the module-level imports at the top of the file
head -30 src/zndraw/app/frame_routes.pyRepository: zincware/ZnDraw
Length of output: 779
🏁 Script executed:
# Check context around line 1108 to verify if it's module-level or function-local
sed -n '1095,1115p' src/zndraw/app/frame_routes.pyRepository: zincware/ZnDraw
Length of output: 603
🏁 Script executed:
# Also verify the exact indentation at line 1108
sed -n '1108p' src/zndraw/app/frame_routes.py | cat -ARepository: zincware/ZnDraw
Length of output: 91
🏁 Script executed:
# Check lines around 271 to confirm it's function-local
sed -n '265,275p' src/zndraw/app/frame_routes.pyRepository: zincware/ZnDraw
Length of output: 469
Move all asebytes imports to module level per coding guidelines.
These imports remain function-local, violating the coding guideline that "Imports should always be at the top of the file" and contradicting the PR's goal to consolidate function-local imports to module-level scope. Additionally, asebytes is imported in three separate functions (lines 271, 1014-1015, 1108), adding overhead on repeated calls.
Consolidate all asebytes imports at the module level:
import msgpack
from flask import Blueprint, Response, current_app, request
+from asebytes import decode as asebytes_decode
+from asebytes import encode as asebytes_encode
+from asebytes import get_metadata
from .frame_index_manager import FrameIndexManagerThen remove the function-local imports at lines 271, 1014-1015, and 1108.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/zndraw/app/frame_routes.py around lines 271, 1014-1015, and 1108,
function-local imports of asebytes are present; move the imports to module level
by adding a single top-of-file import such as "from asebytes import decode as
asebytes_decode, encode as asebytes_encode" (or two separate module-level
imports), then remove the three function-local imports at those line ranges so
the functions use the module-level asebytes_decode and asebytes_encode names.
| # Create test data | ||
| atoms = ase.Atoms("H2O", positions=np.random.rand(3, 3)) | ||
| from asebytes import encode | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate function-local imports to module level.
The from asebytes import encode import is repeated in five test functions (lines 25, 64, 115, 154, 192), and from zndraw.utils import update_colors_and_radii appears in four functions. This contradicts the PR objective to consolidate imports at module scope and violates the coding guideline requiring imports at the top of the file.
🔎 Proposed consolidation
Move these imports to the top of the file with the other module-level imports:
from unittest.mock import Mock, patch
+from asebytes import encode
import ase
import numpy as np
import pytest
from zndraw.zndraw import LocalSettings, ZnDraw
+from zndraw.utils import update_colors_and_radiiThen remove all function-local occurrences at lines 25, 64, 115, 154, and 192.
Based on coding guidelines, imports should always be at the top of the file.
Also applies to: 64-64, 115-115, 154-154, 192-192
🤖 Prompt for AI Agents
In tests/test_chunked_upload_exceptions.py around lines 25, 64, 115, 154, and
192, several tests perform function-local imports of "from asebytes import
encode" and "from zndraw.utils import update_colors_and_radii"; move these
imports to the module-level import block at the top of the file with the other
imports, remove the repeated local imports from those test functions, and run
the test suite to ensure no circular/import-time side effects—if any test relied
on late import for isolation, replace with an explicit fixture or mock instead
of keeping local imports.
| from zndraw.zndraw import ZnDraw | ||
| from unittest.mock import Mock | ||
|
|
||
| from zndraw.zndraw import ZnDraw |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate function-local imports to module level.
Multiple imports (ZnDraw, encode, msgpack, update_colors_and_radii) are repeated across test functions. The AI summary indicates ZnDraw was moved FROM module scope TO function scope, which is the opposite direction of the stated PR objective ("consolidates imports from function-local to module-level scope"). This violates the coding guideline requiring imports at the top of the file.
🔎 Proposed consolidation
Move all imports to the top of the file:
+from unittest.mock import Mock
+
+import msgpack
import ase
import numpy as np
import pytest
-from zndraw.zndraw import LocalSettings
+from asebytes import encode
+from zndraw.utils import update_colors_and_radii
+from zndraw.zndraw import LocalSettings, ZnDrawThen remove all function-local import statements at lines 49-50, 59-61, 77-78, 87-89, 117-120, 128, 147-148, 178-179, 190-192, 212-213, and 224-226.
Based on coding guidelines, imports should always be at the top of the file.
Also applies to: 59-59, 79-79, 87-87, 117-120, 149-149, 180-180, 190-191, 214-214, 224-225
🤖 Prompt for AI Agents
In tests/test_chunked_upload.py around line 51 (and referenced function-local
imports at lines 49-50, 59-61, 77-78, 87-89, 117-120, 128, 147-148, 178-179,
190-192, 212-213, 224-226 and the other repeated locations noted), consolidate
all repeated function-local imports (ZnDraw, encode, msgpack,
update_colors_and_radii, etc.) by moving them to the top of the module and
removing the duplicate imports inside test functions; ensure each symbol is
imported once at module scope and delete the in-function import statements so
tests use the module-level imports.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/hooks/useLazyRoomData.tsapp/src/hooks/usePropertyInspectorSettings.tspyproject.tomlsrc/zndraw/app/settings_routes.pysrc/zndraw/app/utility_routes.pysrc/zndraw/app/worker_routes.pysrc/zndraw/settings.pytests/test_vis_settings.py
✅ Files skipped from review due to trivial changes (1)
- src/zndraw/app/settings_routes.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/zndraw/app/worker_routes.py
- src/zndraw/app/utility_routes.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Docstrings must be concise and to the point
Use type hints wherever possible. Importtyping as tif necessary, but uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
src/zndraw/settings.pytests/test_vis_settings.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test_*.py: Usepytest.mark.parametrizeto avoid code duplication in tests
Tests should be very specific and test only one thing
Avoid complex test setups
Each test must be a function, not a method of a class
Files:
tests/test_vis_settings.py
🧬 Code graph analysis (1)
tests/test_vis_settings.py (1)
tests/conftest.py (1)
server(263-269)
⏰ 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.12, ubuntu-latest)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
🔇 Additional comments (7)
app/src/hooks/useLazyRoomData.ts (1)
2-2: LGTM! Clean removal of unused import.Removing the unused
useQueryClientimport improves code cleanliness and aligns with the PR's objective of cleaning up imports.pyproject.toml (1)
119-120: LGTM! Good addition to enforce import ordering.Enabling Ruff's import-related lint rules aligns well with the PR's objective of standardizing imports across the codebase. The existing codebase already complies with the new rules, ensuring no CI failures going forward.
tests/test_vis_settings.py (1)
16-18: LGTM! Good practice adding timeouts to HTTP requests.Adding explicit timeouts to all HTTP requests prevents tests from hanging indefinitely on network issues or slow server responses. The 10-second timeout is reasonable for test scenarios.
Also applies to: 54-56, 67-67, 72-74, 98-98, 103-105, 124-124
src/zndraw/settings.py (4)
143-148: LGTM! Correct type for sample counts.Changing
min_samplesfromfloattointis semantically appropriate since sample counts are discrete values.
150-156: LGTM! Correct type for sample counts.Changing
samplesfromfloattointis semantically appropriate since sample counts are discrete values.
158-164: LGTM! Correct type for bounce counts.Changing
bouncesfromfloattointis semantically appropriate since bounce counts are discrete values.
166-172: No action needed; the default value was not changed.The tiles field has consistently had a default value of 1 (previously 1.0 as a float type) throughout the git history. The claim that the default changed from 8 to 1 is incorrect. The only change in the current commit is the type adjustment from float to int, which is appropriate since tiles should be an integer value.
Likely an incorrect or invalid review comment.
| // Backend always returns defaults, so once loaded this is guaranteed to exist | ||
| const enabledProperties: string[] = | ||
| isLoading || !settingsResponse | ||
| isLoading || !settingsResponse || !settingsResponse.data.property_inspector | ||
| ? [] | ||
| : settingsResponse.data.property_inspector.enabled_properties; | ||
| : (settingsResponse.data.property_inspector.enabled_properties ?? []); |
There was a problem hiding this comment.
Comment contradicts the defensive null checks.
The comment on line 48 states that "Backend always returns defaults, so once loaded this is guaranteed to exist," yet the new code adds explicit null checks for both property_inspector and enabled_properties. If the backend truly guarantees these fields exist after loading, the defensive checks would be unnecessary. Consider updating the comment to reflect the actual behavior or clarify the defensive programming intent.
🔎 Suggested comment update
- // Extract enabled_properties array from property_inspector settings
- // Backend always returns defaults, so once loaded this is guaranteed to exist
+ // Extract enabled_properties array from property_inspector settings
+ // Defensive null checks ensure safe access even if backend response structure changesCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/hooks/usePropertyInspectorSettings.ts around lines 48 to 52, the
inline comment claims the backend always returns defaults so the value is
guaranteed, but the code contains defensive null checks for property_inspector
and enabled_properties; update the comment to accurately reflect the defensive
intent or the actual backend guarantee (e.g., "Backend generally returns
defaults, but keep defensive null checks in case of missing fields"), so the
comment matches the code behavior and intent.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #827 +/- ##
==========================================
+ Coverage 78.57% 78.68% +0.10%
==========================================
Files 153 153
Lines 18559 18551 -8
==========================================
+ Hits 14582 14596 +14
+ Misses 3977 3955 -22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.