Open
Conversation
Comprehensive review covering god classes, manager duplication, error handling inconsistencies, and CI/CD gaps with a phased refactoring roadmap.
Remove TEST_ERROR_TRIGGER_12345 test block from routes.py and debug print statements from expression_evaluator.py and result_processor.py.
Move AI timeout/limit constants from ai_interface.py and trace limits from action_trace_collector.py into client constants.py. Create static/config.py for server-side paths and schema version.
Replace duplicated load_dotenv patterns in openai_api_base.py, tool_search_service.py, and anthropic_api.py with shared get_api_key() utility. Also remove unused variable in tool_search_service.py.
Replace print() error reporting with logging in routes.py. Add error visibility to bare except blocks. Extract duplicated tool reset and provider model logic into static/route_helpers.py.
Replace >= bounds and unpinned packages with == pins matching the currently installed versions for reproducible builds.
Update test_action_trace_collector.py to import MAX_TRACES from constants instead of the removed _MAX_TRACES private variable. Update test_image_attachment.py to import MAX_ATTACHED_IMAGES and IMAGE_SIZE_WARNING_BYTES from constants instead of accessing them as class attributes. Use Optional[] syntax in route_helpers.py for consistency with codebase conventions.
Move shared telemetry logic (phase tracking, per-drawable counters, frame recording, adapter events) into base_telemetry.py. Canvas2D overrides only _new_drawable_bucket() for its two extra counters. Removes ~270 lines of duplication.
Create a base class that captures the common constructor boilerplate, attribute storage, edit-policy lookup, and name-based retrieval shared across all drawable managers. Migrate PointManager and LabelManager to inherit from it, replacing duplicated __init__ assignments and lookup loops with super().__init__() and _get_by_name() delegation.
- test_env_config.py: 13 tests for load_env_files/get_api_key - test_config.py: 8 tests for server-side constant validation - test_route_helpers.py: 14 tests for provider reset/model/routing helpers
- test_base_drawable_manager.py: 12 tests for constructor, _get_by_name, edit policy resolution, and type filtering - test_base_telemetry.py: 15 test classes covering initialization, reset, timing, recording, snapshot/drain, and Canvas2D bucket override
Replace TEST_ERROR_TRIGGER_12345 magic string with descriptive test message. Tests were already fully mocked and never depended on the server-side trigger.
The _sync_label_position() call mutated state during serialization but was unnecessary — label position is not serialized, and the sync already happens in all mutation methods and the renderer.
Segment, Vector, Circle, Ellipse, Arc, Angle, and Polygon managers now inherit from BaseDrawableManager. Replaces duplicated __init__ boilerplate, edit policy setup, and get_by_name lookups with shared base class behavior. Polygon partially migrated since it manages multiple subtypes dynamically.
Move 10 viewport culling methods from canvas.py into a dedicated VisibilityManager class. Accepts coordinate_mapper and dimensions instead of a Canvas back-reference to avoid circular dependencies. Canvas retains thin delegation stubs to preserve the public API. Reduces canvas.py by ~62 lines.
Add get_by_class_name to drawables container mock so _get_by_name lookups work after AngleManager inherits from BaseDrawableManager.
Move tool call log state and 6 methods into a dedicated ToolCallLogManager class. Update test_tool_call_log.py to test the new class directly. AIInterface delegates via self._tool_call_log instance.
Move message menu, clipboard, and raw text methods into a dedicated MessageMenuManager class. TTS menu items are wired via optional callbacks to avoid coupling. Update test_chat_message_menu.py to test the new class directly.
Move image attachment state and 11 methods into a dedicated ImageAttachmentManager class. System messages routed via callback. AIInterface keeps thin delegation wrappers for external callers.
Move TTS read-aloud, markdown stripping, and settings modal into a dedicated TTSUIManager class. Wired to MessageMenuManager via callbacks for read-aloud and settings menu items.
Move streaming state, message rendering, markdown parsing, and token handlers into a dedicated ChatUIManager class (605 lines). AIInterface drops from 1,547 to 1,078 lines, retaining streaming orchestration, request building, and public API delegation. Cross-concern communication via injected callbacks for timeouts, image clicks, and system messages.
Mark Phases 1-2 as complete, add detailed Phase 3 items covering CI/CD, schema versioning, dependency graph restructuring, workspace migrations, and further decomposition of AIInterface and Canvas.
Update CLAUDE.md backend/client highlights with new modules (config.py, env_config.py, route_helpers.py, 5 extracted AI managers, 3 base classes). Update Project Architecture.txt with restructured AI Interface section, new manager/renderer base classes, and updated file trees.
Migrate remaining load_dotenv patterns in app_manager.py, providers/__init__.py, and openrouter_api.py to use env_config. Replace hardcoded "canvas_snapshots/canvas.png" in openai_api_base.py with config.CANVAS_SNAPSHOT_PATH.
Rename _generate_result_key to generate_result_key in result_processor.py and update caller in tool_call_log_manager.py. Fixes private method coupling flagged in review.
Remove dead code from ai_interface.py (_send_prompt_to_ai_stream, no-op float block). Fix finish_reason typing to Optional[str]. Update image attachment tests to use real ImageAttachmentManager. Extract _get_class_attr to shared simple_mock.py. Standardize details closing, remove redundant import and test.
Drop test_schema_version_at_least_one (redundant with test_schema_version_is_positive_integer). Convert 3 bare assert statements in test_route_helpers.py to self.assertIs().
Document API key required vs optional as deliberate design choice. Remove unused VisibilityManager.update_dimensions. Deduplicate hardcoded voice list via shared TTS_VOICE_OPTIONS constant. Add SvgTelemetry smoke test (6 tests).
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
1 participant
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
Comprehensive architectural refactoring addressing the findings from a full codebase review. This PR covers Phase 1 (quick wins) and Phase 2 (structural improvements) of the refactoring roadmap.
Phase 1: Quick Wins
constants.py, server paths → newconfig.pyload_dotenvpatterns → newenv_config.pyprint()→loggingin routes.py, visibility added to bareexceptblocksroute_helpers.pyPhase 2: Structural Improvements
ToolCallLogManager(258 lines) — tool call visualizationMessageMenuManager(291 lines) — context menus, clipboardImageAttachmentManager(225 lines) — file picker, preview, modalTTSUIManager(222 lines) — read aloud, voice settingsChatUIManager(605 lines) — message rendering, streaming displayNew test coverage
env_config.py,config.py,route_helpers.pyBaseDrawableManager,BaseRendererTelemetryTest plan
🤖 Generated with Claude Code