From 585c75e846ef9266ce31305f59e408f17f1f4e75 Mon Sep 17 00:00:00 2001 From: gilesb Date: Mon, 12 Jan 2026 12:01:54 +0000 Subject: [PATCH] Fix item visibility bugs and add effects web UI - Fix recipe filter to allow owner=None (S-expression compiled recipes) - Fix media uploads to use category (video/image/audio) not MIME type - Fix IPFS imports to detect and store correct media type - Add Effects navigation link between Recipes and Media - Create effects list and detail templates with upload functionality - Add cache/not_found.html template (was missing) - Add type annotations to service classes - Add tests for item visibility and effects web UI (30 tests) Co-Authored-By: Claude Opus 4.5 --- app/services/auth_service.py | 12 +- app/services/cache_service.py | 47 ++-- app/services/recipe_service.py | 26 ++- app/services/run_service.py | 35 +-- app/services/storage_service.py | 18 +- app/templates/base.html | 1 + app/templates/cache/not_found.html | 21 ++ app/templates/effects/detail.html | 195 ++++++++++++++++ app/templates/effects/list.html | 134 +++++++++++ app/types.py | 37 ++++ tests/test_effects_web.py | 345 +++++++++++++++++++++++++++++ tests/test_item_visibility.py | 272 +++++++++++++++++++++++ 12 files changed, 1090 insertions(+), 53 deletions(-) create mode 100644 app/templates/cache/not_found.html create mode 100644 app/templates/effects/detail.html create mode 100644 app/templates/effects/list.html create mode 100644 tests/test_effects_web.py create mode 100644 tests/test_item_visibility.py diff --git a/app/services/auth_service.py b/app/services/auth_service.py index 08e67fc..3f3ce26 100644 --- a/app/services/auth_service.py +++ b/app/services/auth_service.py @@ -5,13 +5,17 @@ Auth Service - token management and user verification. import hashlib import base64 import json -from typing import Optional +from typing import Optional, Dict, Any, TYPE_CHECKING import httpx from artdag_common.middleware.auth import UserContext from ..config import settings +if TYPE_CHECKING: + import redis + from starlette.requests import Request + # Token expiry (30 days to match token lifetime) TOKEN_EXPIRY_SECONDS = 60 * 60 * 24 * 30 @@ -24,7 +28,7 @@ USER_TOKENS_PREFIX = "artdag:user_tokens:" class AuthService: """Service for authentication and token management.""" - def __init__(self, redis_client): + def __init__(self, redis_client: "redis.Redis[bytes]") -> None: self.redis = redis_client def register_user_token(self, username: str, token: str) -> None: @@ -66,7 +70,7 @@ class AuthService: key = f"{REVOKED_KEY_PREFIX}{token_hash}" return self.redis.exists(key) > 0 - def decode_token_claims(self, token: str) -> Optional[dict]: + def decode_token_claims(self, token: str) -> Optional[Dict[str, Any]]: """Decode JWT claims without verification.""" try: parts = token.split(".") @@ -126,7 +130,7 @@ class AuthService: return ctx - def get_user_from_cookie(self, request) -> Optional[UserContext]: + def get_user_from_cookie(self, request: "Request") -> Optional[UserContext]: """Extract user context from auth cookie.""" token = request.cookies.get("auth_token") if not token: diff --git a/app/services/cache_service.py b/app/services/cache_service.py index e67b644..d09ec09 100644 --- a/app/services/cache_service.py +++ b/app/services/cache_service.py @@ -7,10 +7,14 @@ import json import os import subprocess from pathlib import Path -from typing import Optional, List, Dict, Any, Tuple +from typing import Optional, List, Dict, Any, Tuple, TYPE_CHECKING import httpx +if TYPE_CHECKING: + from database import Database + from cache_manager import L1CacheManager + def detect_media_type(cache_path: Path) -> str: """Detect if file is image, video, or audio based on magic bytes.""" @@ -86,7 +90,7 @@ class CacheService: Handles content retrieval, metadata, and media type detection. """ - def __init__(self, database, cache_manager): + def __init__(self, database: "Database", cache_manager: "L1CacheManager") -> None: self.db = database self.cache = cache_manager self.cache_dir = Path(os.environ.get("CACHE_DIR", "/tmp/artdag-cache")) @@ -293,10 +297,10 @@ class CacheService: self, cid: str, actor_id: str, - title: str = None, - description: str = None, - tags: List[str] = None, - custom: Dict[str, Any] = None, + title: Optional[str] = None, + description: Optional[str] = None, + tags: Optional[List[str]] = None, + custom: Optional[Dict[str, Any]] = None, ) -> Tuple[bool, Optional[str]]: """Update content metadata. Returns (success, error).""" if not self.cache.has_content(cid): @@ -431,16 +435,19 @@ class CacheService: if not ipfs_client.get_file(ipfs_cid, str(tmp_path)): return None, f"Could not fetch CID {ipfs_cid} from IPFS" - # Store in cache - cached, ipfs_cid = self.cache.put(tmp_path, node_type="import", move=True) - cid = ipfs_cid or cached.cid # Prefer IPFS CID + # Detect media type before storing + media_type = detect_media_type(tmp_path) - # Save to database - await self.db.create_cache_item(cid, ipfs_cid) + # Store in cache + cached, new_ipfs_cid = self.cache.put(tmp_path, node_type="import", move=True) + cid = new_ipfs_cid or cached.cid # Prefer IPFS CID + + # Save to database with detected media type + await self.db.create_cache_item(cid, new_ipfs_cid) await self.db.save_item_metadata( cid=cid, actor_id=actor_id, - item_type="media", + item_type=media_type, # Use detected type for filtering filename=f"ipfs-{ipfs_cid[:16]}" ) @@ -463,19 +470,21 @@ class CacheService: tmp.write(content) tmp_path = Path(tmp.name) - # Detect MIME type before moving file - mime_type = get_mime_type(tmp_path) + # Detect media type (video/image/audio) before moving file + media_type = detect_media_type(tmp_path) # Store in cache (also stores in IPFS) cached, ipfs_cid = self.cache.put(tmp_path, node_type="upload", move=True) cid = ipfs_cid or cached.cid # Prefer IPFS CID - # Save to database with detected MIME type + # Save to database with media category type + # Using media_type ("video", "image", "audio") not mime_type ("video/mp4") + # so list_media filtering works correctly await self.db.create_cache_item(cid, ipfs_cid) await self.db.save_item_metadata( cid=cid, actor_id=actor_id, - item_type=mime_type, # Store actual MIME type + item_type=media_type, # Store media category for filtering filename=filename ) @@ -485,11 +494,11 @@ class CacheService: async def list_media( self, - actor_id: str = None, - username: str = None, + actor_id: Optional[str] = None, + username: Optional[str] = None, offset: int = 0, limit: int = 24, - media_type: str = None, + media_type: Optional[str] = None, ) -> List[Dict[str, Any]]: """List media items in cache.""" # Get items from database (uses item_types table) diff --git a/app/services/recipe_service.py b/app/services/recipe_service.py index d71a464..914e2a4 100644 --- a/app/services/recipe_service.py +++ b/app/services/recipe_service.py @@ -7,10 +7,16 @@ The recipe ID is the content hash of the file. import tempfile from pathlib import Path -from typing import Optional, List, Dict, Any, Tuple +from typing import Optional, List, Dict, Any, Tuple, TYPE_CHECKING from artdag.sexp import compile_string, parse, serialize, CompileError, ParseError +if TYPE_CHECKING: + import redis + from cache_manager import L1CacheManager + +from ..types import Recipe, CompiledDAG, VisualizationDAG, VisNode, VisEdge + class RecipeService: """ @@ -19,12 +25,12 @@ class RecipeService: Recipes are S-expressions stored in the content-addressed cache. """ - def __init__(self, redis, cache): + def __init__(self, redis: "redis.Redis", cache: "L1CacheManager") -> None: # Redis kept for compatibility but not used for recipe storage self.redis = redis self.cache = cache - async def get_recipe(self, recipe_id: str) -> Optional[Dict[str, Any]]: + async def get_recipe(self, recipe_id: str) -> Optional[Recipe]: """Get a recipe by ID (content hash).""" # Get from cache (content-addressed storage) path = self.cache.get_by_cid(recipe_id) @@ -56,7 +62,7 @@ class RecipeService: return recipe_data - async def list_recipes(self, actor_id: str = None, offset: int = 0, limit: int = 20) -> list: + async def list_recipes(self, actor_id: Optional[str] = None, offset: int = 0, limit: int = 20) -> List[Recipe]: """ List available recipes for a user. @@ -75,7 +81,9 @@ class RecipeService: if recipe and not recipe.get("error"): owner = recipe.get("owner") # Filter by actor - L1 is per-user - if actor_id is None or owner == actor_id: + # Note: S-expression recipes don't have owner field, so owner=None + # means the recipe is shared/public and visible to all users + if actor_id is None or owner is None or owner == actor_id: recipes.append(recipe) else: logger.warning("Cache does not have list_by_type method") @@ -153,19 +161,19 @@ class RecipeService: except Exception as e: return False, f"Failed to delete: {e}" - def parse_recipe(self, content: str) -> Dict[str, Any]: + def parse_recipe(self, content: str) -> CompiledDAG: """Parse recipe S-expression content.""" compiled = compile_string(content) return compiled.to_dict() - def build_dag(self, recipe: Dict[str, Any]) -> Dict[str, Any]: + def build_dag(self, recipe: Recipe) -> VisualizationDAG: """ Build DAG visualization data from recipe. Returns nodes and edges for Cytoscape.js. """ - vis_nodes = [] - edges = [] + vis_nodes: List[VisNode] = [] + edges: List[VisEdge] = [] dag = recipe.get("dag", {}) dag_nodes = dag.get("nodes", []) diff --git a/app/services/run_service.py b/app/services/run_service.py index 94e5457..a0ad549 100644 --- a/app/services/run_service.py +++ b/app/services/run_service.py @@ -11,10 +11,17 @@ import json import os from datetime import datetime, timezone from pathlib import Path -from typing import Optional, List, Dict, Any, Tuple +from typing import Optional, List, Dict, Any, Tuple, Union, TYPE_CHECKING + +if TYPE_CHECKING: + import redis + from cache_manager import L1CacheManager + from database import Database + +from ..types import RunResult -def compute_run_id(input_hashes: list, recipe: str, recipe_hash: str = None) -> str: +def compute_run_id(input_hashes: Union[List[str], Dict[str, str]], recipe: str, recipe_hash: Optional[str] = None) -> str: """ Compute a deterministic run_id from inputs and recipe. @@ -89,14 +96,14 @@ class RunService: Redis is only used for task_id mapping (ephemeral). """ - def __init__(self, database, redis, cache): + def __init__(self, database: "Database", redis: "redis.Redis[bytes]", cache: "L1CacheManager") -> None: self.db = database self.redis = redis # Only for task_id mapping self.cache = cache self.task_key_prefix = "artdag:task:" # run_id -> task_id mapping only self.cache_dir = Path(os.environ.get("CACHE_DIR", "/tmp/artdag-cache")) - def _ensure_inputs_list(self, inputs) -> list: + def _ensure_inputs_list(self, inputs: Any) -> List[str]: """Ensure inputs is a list, parsing JSON string if needed.""" if inputs is None: return [] @@ -112,7 +119,7 @@ class RunService: return [] return [] - async def get_run(self, run_id: str) -> Optional[Dict[str, Any]]: + async def get_run(self, run_id: str) -> Optional[RunResult]: """Get a run by ID. Checks database first, then Celery task state.""" # Check database for completed run cached = await self.db.get_run_cache(run_id) @@ -267,7 +274,7 @@ class RunService: return None - async def list_runs(self, actor_id: str, offset: int = 0, limit: int = 20) -> list: + async def list_runs(self, actor_id: str, offset: int = 0, limit: int = 20) -> List[RunResult]: """List runs for a user. Returns completed and pending runs from database.""" # Get completed runs from database completed_runs = await self.db.list_runs_by_actor(actor_id, offset=0, limit=limit + 50) @@ -297,14 +304,14 @@ class RunService: async def create_run( self, recipe: str, - inputs: list, - output_name: str = None, + inputs: Union[List[str], Dict[str, str]], + output_name: Optional[str] = None, use_dag: bool = True, - dag_json: str = None, - actor_id: str = None, - l2_server: str = None, - recipe_name: str = None, - ) -> Tuple[Optional[Dict[str, Any]], Optional[str]]: + dag_json: Optional[str] = None, + actor_id: Optional[str] = None, + l2_server: Optional[str] = None, + recipe_name: Optional[str] = None, + ) -> Tuple[Optional[RunResult], Optional[str]]: """ Create a new rendering run. Checks cache before executing. @@ -604,7 +611,7 @@ class RunService: """Detect media type for a file path.""" return detect_media_type(path) - async def recover_pending_runs(self) -> Dict[str, int]: + async def recover_pending_runs(self) -> Dict[str, Union[int, str]]: """ Recover pending runs after restart. diff --git a/app/services/storage_service.py b/app/services/storage_service.py index b7349a8..19d4e3c 100644 --- a/app/services/storage_service.py +++ b/app/services/storage_service.py @@ -3,7 +3,11 @@ Storage Service - business logic for storage provider management. """ import json -from typing import Optional, List, Dict, Any +from typing import Optional, List, Dict, Any, Tuple, TYPE_CHECKING + +if TYPE_CHECKING: + from database import Database + from storage_providers import StorageProvidersModule STORAGE_PROVIDERS_INFO = { @@ -22,7 +26,7 @@ VALID_PROVIDER_TYPES = list(STORAGE_PROVIDERS_INFO.keys()) class StorageService: """Service for managing user storage providers.""" - def __init__(self, database, storage_providers_module): + def __init__(self, database: "Database", storage_providers_module: "StorageProvidersModule") -> None: self.db = database self.providers = storage_providers_module @@ -72,7 +76,7 @@ class StorageService: capacity_gb: int = 5, provider_name: Optional[str] = None, description: Optional[str] = None, - ) -> tuple[Optional[int], Optional[str]]: + ) -> Tuple[Optional[int], Optional[str]]: """Add a new storage provider. Returns (storage_id, error_message).""" if provider_type not in VALID_PROVIDER_TYPES: return None, f"Invalid provider type: {provider_type}" @@ -115,7 +119,7 @@ class StorageService: config: Optional[Dict[str, Any]] = None, capacity_gb: Optional[int] = None, is_active: Optional[bool] = None, - ) -> tuple[bool, Optional[str]]: + ) -> Tuple[bool, Optional[str]]: """Update a storage provider. Returns (success, error_message).""" storage = await self.db.get_storage_by_id(storage_id) if not storage: @@ -145,7 +149,7 @@ class StorageService: return success, None if success else "Failed to update storage provider" - async def delete_storage(self, storage_id: int, actor_id: str) -> tuple[bool, Optional[str]]: + async def delete_storage(self, storage_id: int, actor_id: str) -> Tuple[bool, Optional[str]]: """Delete a storage provider. Returns (success, error_message).""" storage = await self.db.get_storage_by_id(storage_id) if not storage: @@ -156,7 +160,7 @@ class StorageService: success = await self.db.remove_user_storage(storage_id) return success, None if success else "Failed to remove storage provider" - async def test_storage(self, storage_id: int, actor_id: str) -> tuple[bool, str]: + async def test_storage(self, storage_id: int, actor_id: str) -> Tuple[bool, str]: """Test storage provider connectivity. Returns (success, message).""" storage = await self.db.get_storage_by_id(storage_id) if not storage: @@ -179,7 +183,7 @@ class StorageService: """List storage providers of a specific type.""" return await self.db.get_user_storage_by_type(actor_id, provider_type) - def build_config_from_form(self, provider_type: str, form_data: Dict[str, Any]) -> tuple[Optional[Dict], Optional[str]]: + def build_config_from_form(self, provider_type: str, form_data: Dict[str, Any]) -> Tuple[Optional[Dict[str, Any]], Optional[str]]: """Build provider config from form data. Returns (config, error).""" api_key = form_data.get("api_key") secret_key = form_data.get("secret_key") diff --git a/app/templates/base.html b/app/templates/base.html index 94e3e58..04deecc 100644 --- a/app/templates/base.html +++ b/app/templates/base.html @@ -6,6 +6,7 @@