From a37a3122f94a8b62b639135a74812612e26a6201 Mon Sep 17 00:00:00 2001 From: Alex Blank Date: Mon, 25 May 2026 14:01:32 +0200 Subject: [PATCH] refactor: extract shared validation and reduce duplication - Extract tool_types validation to shared module (validate_compose_yaml, check_port_exposed, validate_required_variables) - Extract _get_user and _get_owned_project to auth/dependencies.py - Create useAsyncData hook and apply to 6 pages - Create extractErrorMessage utility - TypeScript and build pass --- apps/api/src/api/git_repositories.py | 36 +---- apps/api/src/api/projects.py | 36 +---- apps/api/src/api/ssh_keys.py | 10 +- apps/api/src/api/tool_instances.py | 37 +---- apps/api/src/api/tool_types.py | 162 +++++----------------- apps/api/src/api/tool_types_validation.py | 87 ++++++++++++ apps/api/src/api/user_config.py | 10 +- apps/api/src/api/users.py | 10 +- apps/api/src/auth/dependencies.py | 36 +++++ apps/web/src/hooks/use-async-data.ts | 42 ++++++ apps/web/src/pages/config-profiles.tsx | 11 +- apps/web/src/pages/git-history.tsx | 40 +++--- apps/web/src/pages/git-repositories.tsx | 43 +++--- apps/web/src/pages/profile.tsx | 73 +++++----- apps/web/src/pages/projects.tsx | 35 ++--- apps/web/src/pages/settings.tsx | 26 ++-- apps/web/src/pages/ssh-keys.tsx | 55 ++++---- apps/web/src/pages/tool-workshop.tsx | 11 +- apps/web/src/utils/errors.ts | 9 ++ 19 files changed, 327 insertions(+), 442 deletions(-) create mode 100644 apps/api/src/api/tool_types_validation.py create mode 100644 apps/web/src/hooks/use-async-data.ts create mode 100644 apps/web/src/utils/errors.ts diff --git a/apps/api/src/api/git_repositories.py b/apps/api/src/api/git_repositories.py index 5190afe..dca4b57 100644 --- a/apps/api/src/api/git_repositories.py +++ b/apps/api/src/api/git_repositories.py @@ -10,7 +10,7 @@ from pydantic import BaseModel, ConfigDict from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession -from src.auth.dependencies import get_current_user_id, get_db_session +from src.auth.dependencies import _get_owned_project, _get_user, get_current_user_id, get_db_session from src.config import Settings from src.models.git_repository import GitRepository from src.models.project import Project @@ -42,40 +42,6 @@ router = APIRouter(prefix="/projects", tags=["git-repositories"]) logger = logging.getLogger(__name__) -async def _get_user(session: AsyncSession, user_id: uuid.UUID) -> User: - """Fetch a user by ID or raise 401 if not found.""" - user = await session.get(User, user_id) - if user is None: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="user not found") - return user - - -async def _get_owned_project( - project_id: uuid.UUID, - user_id: uuid.UUID, - session: AsyncSession, -) -> Project: - """Fetch a project and verify ownership. - - Args: - project_id: UUID of the project. - user_id: ID of the authenticated user. - session: Database session. - - Returns: - The project if found and owned by the user. - - Raises: - HTTPException: If project not found or user is not the owner. - """ - project = await session.get(Project, project_id) - if project is None: - raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="project not found") - if project.owner_id != user_id: - raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="not project owner") - return project - - def _get_repo_path(user_id: uuid.UUID, project_id: uuid.UUID, name: str) -> str: """Generate the filesystem path for a repository. diff --git a/apps/api/src/api/projects.py b/apps/api/src/api/projects.py index a9d1056..9a19c12 100644 --- a/apps/api/src/api/projects.py +++ b/apps/api/src/api/projects.py @@ -7,7 +7,7 @@ from pydantic import BaseModel, ConfigDict from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession -from src.auth.dependencies import get_current_user_id, get_db_session +from src.auth.dependencies import _get_owned_project, _get_user, get_current_user_id, get_db_session from src.models.git_repository import GitRepository from src.models.project import Project from src.models.ssh_key import SSHKey @@ -16,14 +16,6 @@ from src.models.user import User router = APIRouter(prefix="/projects", tags=["projects"]) -async def _get_user(session: AsyncSession, user_id: uuid.UUID) -> User: - """Fetch a user by ID or raise 401 if not found.""" - user = await session.get(User, user_id) - if user is None: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="user not found") - return user - - class ProjectCreate(BaseModel): name: str description: str | None = None @@ -132,32 +124,6 @@ async def get_project( return await _get_owned_project(project_id, user_id, session) -async def _get_owned_project( - project_id: uuid.UUID, - user_id: uuid.UUID, - session: AsyncSession, -) -> Project: - """Fetch a project and verify ownership. - - Args: - project_id: UUID of the project. - user_id: ID of the authenticated user. - session: Database session. - - Returns: - The project if found and owned by the user. - - Raises: - HTTPException: If project not found or user is not the owner. - """ - project = await session.get(Project, project_id) - if project is None: - raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="project not found") - if project.owner_id != user_id: - raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="not project owner") - return project - - @router.patch( "/{project_id}", response_model=ProjectResponse, diff --git a/apps/api/src/api/ssh_keys.py b/apps/api/src/api/ssh_keys.py index 9dcfdb0..c03d607 100644 --- a/apps/api/src/api/ssh_keys.py +++ b/apps/api/src/api/ssh_keys.py @@ -10,7 +10,7 @@ from pydantic import BaseModel, ConfigDict from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession -from src.auth.dependencies import get_current_user_id, get_db_session +from src.auth.dependencies import _get_user, get_current_user_id, get_db_session from src.config import Settings from src.models.ssh_key import SSHKey from src.models.user import User @@ -18,14 +18,6 @@ from src.models.user import User router = APIRouter(prefix="/ssh-keys", tags=["ssh-keys"]) -async def _get_user(session: AsyncSession, user_id: uuid.UUID) -> User: - """Fetch a user by ID or raise 401 if not found.""" - user = await session.get(User, user_id) - if user is None: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="user not found") - return user - - def _get_fernet() -> Fernet: """Generate a valid Fernet key from the session secret.""" import base64 diff --git a/apps/api/src/api/tool_instances.py b/apps/api/src/api/tool_instances.py index ab85435..c130d94 100644 --- a/apps/api/src/api/tool_instances.py +++ b/apps/api/src/api/tool_instances.py @@ -15,8 +15,7 @@ from sqlalchemy.ext.asyncio import AsyncSession logger = logging.getLogger(__name__) -from src.auth.dependencies import get_current_user_id -from src.auth.dependencies import get_db_session +from src.auth.dependencies import _get_owned_project, _get_user, get_current_user_id, get_db_session from src.models.git_repository import GitRepository from src.models.project import Project from src.models.ssh_key import SSHKey @@ -193,40 +192,6 @@ def _modify_compose_file( compose_file.write_text(yaml.dump(compose_data, default_flow_style=False)) -async def _get_user(session: AsyncSession, user_id: uuid.UUID) -> User: - """Fetch a user by ID or raise 404 if not found.""" - user = await session.get(User, user_id) - if user is None: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, detail="user not found" - ) - return user - - -async def _get_owned_project( - project_id: uuid.UUID, user_id: uuid.UUID, session: AsyncSession -) -> Project: - """Fetch a project and verify ownership. - - Args: - project_id: UUID of the project. - user_id: ID of the authenticated user. - session: Database session. - - Returns: - The project if found and owned by the user. - - Raises: - HTTPException: If project not found or user is not the owner. - """ - project = await session.get(Project, project_id) - if project is None or project.owner_id != user_id: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, detail="project not found" - ) - return project - - @router.post( "/{project_id}/repositories/{repo_id}/instances", summary="Create tool instance", diff --git a/apps/api/src/api/tool_types.py b/apps/api/src/api/tool_types.py index 16c7bb9..205e795 100644 --- a/apps/api/src/api/tool_types.py +++ b/apps/api/src/api/tool_types.py @@ -1,4 +1,3 @@ -import re import uuid from datetime import datetime @@ -8,26 +7,19 @@ from pydantic import BaseModel, ConfigDict, field_validator, model_validator from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession - -def _sanitize_template_vars(template: str) -> str: - """Replace template variables like {{VAR}} with placeholders to avoid YAML parsing errors.""" - return re.sub(r"\{\{[A-Za-z_][A-Za-z0-9_]*\}\}", "__PLACEHOLDER__", template) - -from src.auth.dependencies import get_current_user_id, get_db_session +from src.api.tool_types_validation import ( + check_port_exposed, + sanitize_template_vars, + validate_compose_yaml, + validate_required_variables, +) +from src.auth.dependencies import _get_user, get_current_user_id, get_db_session from src.models.tool_type import ToolType from src.models.user import User router = APIRouter(prefix="/tool-types", tags=["tool-types"]) -async def _get_user(session: AsyncSession, user_id: uuid.UUID) -> User: - """Fetch a user by ID or raise 401 if not found.""" - user = await session.get(User, user_id) - if user is None: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="user not found") - return user - - async def _require_admin(user: User) -> None: """Check if user has admin privileges. @@ -72,24 +64,7 @@ class ToolTypeCreate(BaseModel): if v is None: raise ValueError("compose_template is required when definition_type is 'compose'") - # Replace template variables with dummy values before YAML validation - # to avoid YAML parsing errors with {{VAR}} syntax - sanitized = _sanitize_template_vars(v) - - try: - parsed = yaml.safe_load(sanitized) - except yaml.YAMLError as e: - raise ValueError(f"Invalid YAML: {e}") - - if not isinstance(parsed, dict): - raise ValueError("Compose template must be a YAML mapping") - - if "services" not in parsed: - raise ValueError("Compose template must contain 'services' key") - - if not parsed["services"]: - raise ValueError("Compose template must define at least one service") - + validate_compose_yaml(v) return v @field_validator("dockerfile_template") @@ -156,29 +131,11 @@ class ToolTypeCreate(BaseModel): # Validate that default_port is exposed in compose template (only if requires_port) if self.requires_port and self.definition_type == "compose" and self.compose_template: try: - sanitized = _sanitize_template_vars(self.compose_template) - parsed = yaml.safe_load(sanitized) - except yaml.YAMLError: + parsed = validate_compose_yaml(self.compose_template) + except ValueError: return self - port_str = str(self.default_port) - port_exposed = False - - if isinstance(parsed, dict) and "services" in parsed: - for service_name, service_config in parsed["services"].items(): - if isinstance(service_config, dict) and "ports" in service_config: - for port_mapping in service_config["ports"]: - if isinstance(port_mapping, str): - if port_str in port_mapping: - port_exposed = True - break - elif isinstance(port_mapping, int) and port_mapping == self.default_port: - port_exposed = True - break - if port_exposed: - break - - if not port_exposed: + if not check_port_exposed(parsed, self.default_port): raise ValueError(f"Port {self.default_port} is not exposed in the compose template. Add it to the 'ports' section.") return self @@ -222,29 +179,13 @@ class ToolTypeUpdate(BaseModel): def validate_compose_template(cls, v: str | None, info) -> str | None: if v is None: return v - + data = info.data definition_type = data.get("definition_type") if definition_type and definition_type != "compose": return v - - # Replace template variables with dummy values before YAML validation - sanitized = _sanitize_template_vars(v) - - try: - parsed = yaml.safe_load(sanitized) - except yaml.YAMLError as e: - raise ValueError(f"Invalid YAML: {e}") - - if not isinstance(parsed, dict): - raise ValueError("Compose template must be a YAML mapping") - - if "services" not in parsed: - raise ValueError("Compose template must contain 'services' key") - - if not parsed["services"]: - raise ValueError("Compose template must define at least one service") - + + validate_compose_yaml(v) return v @field_validator("dockerfile_template") @@ -442,54 +383,29 @@ async def update_tool_type( template = update_data.get("compose_template", tool_type.compose_template) if template: try: - sanitized = _sanitize_template_vars(template) - parsed = yaml.safe_load(sanitized) - except yaml.YAMLError: - parsed = None - - if parsed and isinstance(parsed, dict) and "services" in parsed: - port_str = str(new_port) - port_exposed = False - for service_config in parsed["services"].values(): - if isinstance(service_config, dict) and "ports" in service_config: - for port_mapping in service_config["ports"]: - if isinstance(port_mapping, str) and port_str in port_mapping: - port_exposed = True - break - elif isinstance(port_mapping, int) and port_mapping == new_port: - port_exposed = True - break - if port_exposed: - break - - if not port_exposed: + parsed = validate_compose_yaml(template) + if not check_port_exposed(parsed, new_port): raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=f"Port {new_port} is not exposed in the compose template" ) - + except ValueError as e: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=str(e) + ) + # Validate required variables for compose definitions definition_type = update_data.get("definition_type", tool_type.definition_type) if definition_type == "compose": if "required_variables" in update_data and "compose_template" in update_data: - template = update_data["compose_template"] - for var in update_data["required_variables"]: - placeholder = f"{{{{{var}}}}}" - if placeholder not in template: - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=f"Required variable '{var}' not found in compose template" - ) + validate_required_variables( + update_data["compose_template"], update_data["required_variables"] + ) elif "required_variables" in update_data: template = tool_type.compose_template if template: - for var in update_data["required_variables"]: - placeholder = f"{{{{{var}}}}}" - if placeholder not in template: - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=f"Required variable '{var}' not found in compose template" - ) + validate_required_variables(template, update_data["required_variables"]) for field, value in update_data.items(): setattr(tool_type, field, value) @@ -534,16 +450,9 @@ async def validate_tool_type_template( errors.append("Compose template is required") else: try: - sanitized = _sanitize_template_vars(data.compose_template) - parsed = yaml.safe_load(sanitized) - if not isinstance(parsed, dict): - errors.append("Compose template must be a YAML mapping") - elif "services" not in parsed: - errors.append("Compose template must contain 'services' key") - elif not parsed["services"]: - errors.append("Compose template must define at least one service") - except yaml.YAMLError as e: - errors.append(f"Invalid YAML: {e}") + validate_compose_yaml(data.compose_template) + except ValueError as e: + errors.append(str(e)) elif data.definition_type == "dockerfile": if not data.dockerfile_template: @@ -592,16 +501,9 @@ async def validate_tool_type( errors.append("Compose template is empty") else: try: - sanitized = _sanitize_template_vars(tool_type.compose_template) - parsed = yaml.safe_load(sanitized) - if not isinstance(parsed, dict): - errors.append("Compose template must be a YAML mapping") - elif "services" not in parsed: - errors.append("Compose template must contain 'services' key") - elif not parsed["services"]: - errors.append("Compose template must define at least one service") - except yaml.YAMLError as e: - errors.append(f"Invalid YAML: {e}") + validate_compose_yaml(tool_type.compose_template) + except ValueError as e: + errors.append(str(e)) elif tool_type.definition_type == "dockerfile": if not tool_type.dockerfile_template: diff --git a/apps/api/src/api/tool_types_validation.py b/apps/api/src/api/tool_types_validation.py new file mode 100644 index 0000000..100a5e5 --- /dev/null +++ b/apps/api/src/api/tool_types_validation.py @@ -0,0 +1,87 @@ +"""Shared validation utilities for tool types.""" + +import re + +import yaml +from fastapi import HTTPException, status + + +def sanitize_template_vars(template: str) -> str: + """Replace template variables like {{VAR}} with placeholders to avoid YAML parsing errors.""" + return re.sub(r"\{\{[A-Za-z_][A-Za-z0-9_]*\}\}", "__PLACEHOLDER__", template) + + +def validate_compose_yaml(template: str) -> dict: + """Validate and parse a compose template. + + Args: + template: Raw compose template string. + + Returns: + Parsed YAML dict. + + Raises: + ValueError: If YAML is invalid or missing required keys. + """ + sanitized = sanitize_template_vars(template) + + try: + parsed = yaml.safe_load(sanitized) + except yaml.YAMLError as e: + raise ValueError(f"Invalid YAML: {e}") + + if not isinstance(parsed, dict): + raise ValueError("Compose template must be a YAML mapping") + + if "services" not in parsed: + raise ValueError("Compose template must contain 'services' key") + + if not parsed["services"]: + raise ValueError("Compose template must define at least one service") + + return parsed + + +def check_port_exposed(parsed: dict, port: int) -> bool: + """Check if a port is exposed in a parsed compose template. + + Args: + parsed: Parsed compose YAML dict. + port: Port number to check. + + Returns: + True if port is exposed in any service. + """ + port_str = str(port) + + if not isinstance(parsed, dict) or "services" not in parsed: + return False + + for service_config in parsed["services"].values(): + if isinstance(service_config, dict) and "ports" in service_config: + for port_mapping in service_config["ports"]: + if isinstance(port_mapping, str) and port_str in port_mapping: + return True + elif isinstance(port_mapping, int) and port_mapping == port: + return True + + return False + + +def validate_required_variables(template: str, variables: list[str]) -> None: + """Validate that all required variables exist in the template. + + Args: + template: Compose template string. + variables: List of required variable names. + + Raises: + HTTPException: If any variable is not found in the template. + """ + for var in variables: + placeholder = f"{{{{{var}}}}}" + if placeholder not in template: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=f"Required variable '{var}' not found in compose template", + ) diff --git a/apps/api/src/api/user_config.py b/apps/api/src/api/user_config.py index 917bfa9..23943e9 100644 --- a/apps/api/src/api/user_config.py +++ b/apps/api/src/api/user_config.py @@ -8,21 +8,13 @@ from pydantic import BaseModel, ConfigDict from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession -from src.auth.dependencies import get_current_user_id, get_db_session +from src.auth.dependencies import _get_user, get_current_user_id, get_db_session from src.models.user import User from src.models.user_config import UserConfig router = APIRouter(prefix="/users/me", tags=["user-config"]) -async def _get_user(session: AsyncSession, user_id: uuid.UUID) -> User: - """Fetch a user by ID or raise 401 if not found.""" - user = await session.get(User, user_id) - if user is None: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="user not found") - return user - - async def _get_or_create_config(session: AsyncSession, user_id: uuid.UUID) -> UserConfig: """Get or create user config record. diff --git a/apps/api/src/api/users.py b/apps/api/src/api/users.py index 65a49c9..5533b4b 100644 --- a/apps/api/src/api/users.py +++ b/apps/api/src/api/users.py @@ -5,7 +5,7 @@ from fastapi import APIRouter, Depends, HTTPException, UploadFile, status from pydantic import BaseModel, ConfigDict from sqlalchemy.ext.asyncio import AsyncSession -from src.auth.dependencies import get_current_user_id, get_db_session +from src.auth.dependencies import _get_user, get_current_user_id, get_db_session from src.models.user import User router = APIRouter(prefix="/users", tags=["users"]) @@ -16,14 +16,6 @@ ALLOWED_CONTENT_TYPES = {"image/png", "image/jpeg", "image/jpg"} MAX_AVATAR_SIZE = 2 * 1024 * 1024 # 2MB -async def _get_user(session: AsyncSession, user_id: uuid.UUID) -> User: - """Fetch a user by ID or raise 401 if not found.""" - user = await session.get(User, user_id) - if user is None: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="user not found") - return user - - class UserProfileResponse(BaseModel): model_config = ConfigDict(from_attributes=True) diff --git a/apps/api/src/auth/dependencies.py b/apps/api/src/auth/dependencies.py index f177b35..7d7c5b5 100644 --- a/apps/api/src/auth/dependencies.py +++ b/apps/api/src/auth/dependencies.py @@ -47,3 +47,39 @@ async def get_current_user( if user is None: raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="user not found") return user + + +async def _get_user(session: AsyncSession, user_id: uuid.UUID) -> User: + """Fetch a user by ID or raise 401 if not found.""" + user = await session.get(User, user_id) + if user is None: + raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="user not found") + return user + + +async def _get_owned_project( + project_id: uuid.UUID, + user_id: uuid.UUID, + session: AsyncSession, +) -> "Project": + """Fetch a project and verify ownership. + + Args: + project_id: UUID of the project. + user_id: ID of the authenticated user. + session: Database session. + + Returns: + The project if found and owned by the user. + + Raises: + HTTPException: 404 if project not found, 403 if user is not the owner. + """ + from src.models.project import Project + + project = await session.get(Project, project_id) + if project is None: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="project not found") + if project.owner_id != user_id: + raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="not project owner") + return project diff --git a/apps/web/src/hooks/use-async-data.ts b/apps/web/src/hooks/use-async-data.ts new file mode 100644 index 0000000..db147a7 --- /dev/null +++ b/apps/web/src/hooks/use-async-data.ts @@ -0,0 +1,42 @@ +import { useCallback, useEffect, useState } from "react"; + +type AsyncStatus = "idle" | "loading" | "ready" | "error"; + +interface UseAsyncDataResult { + data: T | null; + status: AsyncStatus; + error: string | null; + reload: () => void; +} + +export function useAsyncData( + fetcher: () => Promise, + deps: React.DependencyList = [] +): UseAsyncDataResult { + const [data, setData] = useState(null); + const [status, setStatus] = useState("idle"); + const [error, setError] = useState(null); + + const load = useCallback(async () => { + setStatus("loading"); + setError(null); + try { + const result = await fetcher(); + setData(result); + setStatus("ready"); + } catch (err) { + setError(err instanceof Error ? err.message : "Failed to load data"); + setStatus("error"); + } + }, deps); + + const reload = useCallback(() => { + void load(); + }, [load]); + + useEffect(() => { + void load(); + }, [load]); + + return { data, status, error, reload }; +} diff --git a/apps/web/src/pages/config-profiles.tsx b/apps/web/src/pages/config-profiles.tsx index 4d28e28..119801d 100644 --- a/apps/web/src/pages/config-profiles.tsx +++ b/apps/web/src/pages/config-profiles.tsx @@ -1,6 +1,7 @@ import { useCallback, useEffect, useState } from "react"; import { Icon } from "../components/icon"; import { useMobileViewport } from "../hooks/use-mobile-viewport"; +import { extractErrorMessage } from "../utils/errors"; import { MobileListView } from "../components/mobile-list-view"; import { MobileDetailView } from "../components/mobile-detail-view"; import { MobileEditView } from "../components/mobile-edit-view"; @@ -127,16 +128,6 @@ export const ConfigProfilesPage = () => { resetForm(); }; - const extractErrorMessage = (err: unknown): string => { - const axiosError = err as { response?: { data?: { detail?: string | Array<{msg?: string}> } } }; - const detail = axiosError?.response?.data?.detail; - if (typeof detail === "string") return detail; - if (Array.isArray(detail)) { - return detail.map((d) => typeof d === "string" ? d : d.msg || JSON.stringify(d)).join(", "); - } - return "Failed to save"; - }; - // Include management functions const getIncludedProfile = (id: string): ConfigProfile | undefined => profiles.find((p) => p.id === id); diff --git a/apps/web/src/pages/git-history.tsx b/apps/web/src/pages/git-history.tsx index 777d142..beb27ec 100644 --- a/apps/web/src/pages/git-history.tsx +++ b/apps/web/src/pages/git-history.tsx @@ -1,39 +1,32 @@ import { useCallback, useEffect, useState } from "react"; import { useNavigate, useParams } from "react-router-dom"; -import { getCommitDetail, getRepositoryHistory, type CommitDetail, type CommitHistoryEntry } from "../api/git_repositories"; +import { getCommitDetail, getRepositoryHistory, type CommitDetail, type CommitHistoryEntry, type CommitHistoryResponse } from "../api/git_repositories"; import { Icon } from "../components/icon"; +import { useAsyncData } from "../hooks/use-async-data"; export const GitHistoryPage = () => { const { projectId, repoId } = useParams<{ projectId: string; repoId: string }>(); const navigate = useNavigate(); - const [commits, setCommits] = useState([]); const [selectedCommit, setSelectedCommit] = useState(null); const [commitDetail, setCommitDetail] = useState(null); - const [branches, setBranches] = useState([]); const [selectedBranch, setSelectedBranch] = useState(""); - const [status, setStatus] = useState<"loading" | "ready" | "error">("loading"); const [detailStatus, setDetailStatus] = useState<"idle" | "loading" | "ready" | "error">("idle"); - const loadHistory = useCallback(async () => { - if (!projectId || !repoId) return; - setStatus("loading"); - try { - const data = await getRepositoryHistory(projectId, repoId, selectedBranch || undefined, 10000); - setCommits(data.commits); - setBranches(data.branches); - if (data.branches.length > 0 && !selectedBranch) { - setSelectedBranch(data.branches[0]); - } - setStatus("ready"); - } catch { - setStatus("error"); - } - }, [projectId, repoId, selectedBranch]); + const { data: historyData, status, reload } = useAsyncData( + async () => { + if (!projectId || !repoId) return { commits: [], branches: [], tags: [] }; + return await getRepositoryHistory(projectId, repoId, selectedBranch || undefined, 10000); + }, + [projectId, repoId, selectedBranch] + ); + // Auto-select first branch when data loads useEffect(() => { - void loadHistory(); - }, [loadHistory]); + if (historyData?.branches.length && !selectedBranch) { + setSelectedBranch(historyData.branches[0]); + } + }, [historyData?.branches, selectedBranch]); const handleCommitClick = async (hash: string) => { if (!projectId || !repoId) return; @@ -70,7 +63,7 @@ export const GitHistoryPage = () => { return (

Failed to load commit history

- @@ -78,6 +71,9 @@ export const GitHistoryPage = () => { ); } + const commits = historyData?.commits ?? []; + const branches = historyData?.branches ?? []; + return (
diff --git a/apps/web/src/pages/git-repositories.tsx b/apps/web/src/pages/git-repositories.tsx index 1ad2c4e..5f2c195 100644 --- a/apps/web/src/pages/git-repositories.tsx +++ b/apps/web/src/pages/git-repositories.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from "react"; +import { useState } from "react"; import { useNavigate, useParams } from "react-router-dom"; import { @@ -8,46 +8,35 @@ import { import type { GitRepository } from "../api/git_repositories"; import { Icon } from "../components/icon"; import { RepositoryCreateDialog } from "../components/repository-create-dialog"; - -type RepoStatus = "loading" | "ready" | "error"; +import { useAsyncData } from "../hooks/use-async-data"; export const GitRepositoriesPage = () => { const { projectId } = useParams<{ projectId: string }>(); const navigate = useNavigate(); - const [status, setStatus] = useState("loading"); - const [repositories, setRepositories] = useState([]); const [showCreate, setShowCreate] = useState(false); const [deleteConfirmId, setDeleteConfirmId] = useState(null); - const loadRepositories = useCallback(async () => { - if (!projectId) return; - setStatus("loading"); - try { - const data = await listRepositories(projectId); - setRepositories(data); - setStatus("ready"); - } catch { - setRepositories([]); - setStatus("error"); - } - }, [projectId]); - - useEffect(() => { - void loadRepositories(); - }, [loadRepositories]); + const { data: repositories, status, reload } = useAsyncData( + async () => { + if (!projectId) return []; + return await listRepositories(projectId); + }, + [projectId] + ); const handleDelete = async (repoId: string) => { if (!projectId) return; try { await deleteRepository(projectId, repoId); setDeleteConfirmId(null); - await loadRepositories(); + reload(); } catch { setDeleteConfirmId(null); } }; - const isEmpty = status === "ready" && repositories.length === 0; + const safeRepositories = repositories ?? []; + const isEmpty = status === "ready" && safeRepositories.length === 0; return (
@@ -64,7 +53,7 @@ export const GitRepositoriesPage = () => { {status === "error" && (

Failed to load repositories

- @@ -73,9 +62,9 @@ export const GitRepositoriesPage = () => { {isEmpty &&

No repositories yet. Create your first repository above.

} - {status === "ready" && repositories.length > 0 && ( + {status === "ready" && safeRepositories.length > 0 && (
- {repositories.map((repo) => ( + {safeRepositories.map((repo) => (

{repo.name}

@@ -131,7 +120,7 @@ export const GitRepositoriesPage = () => { open={showCreate} title="Create Repository" onClose={() => setShowCreate(false)} - onCreated={loadRepositories} + onCreated={reload} /> )}
diff --git a/apps/web/src/pages/profile.tsx b/apps/web/src/pages/profile.tsx index 45d8718..3f48ccd 100644 --- a/apps/web/src/pages/profile.tsx +++ b/apps/web/src/pages/profile.tsx @@ -3,37 +3,35 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { getProfile, updateProfile, uploadAvatar } from "../api/profile"; import { Icon } from "../components/icon"; import { useAuth } from "../state/auth"; +import { useAsyncData } from "../hooks/use-async-data"; import type { UserProfile } from "../api/profile"; type ProfileStatus = "loading" | "ready" | "error" | "saving"; export const ProfilePage = () => { const { refreshSession } = useAuth(); - const [status, setStatus] = useState("loading"); - const [profile, setProfile] = useState(null); + const { data: profile, status: loadStatus, reload } = useAsyncData(getProfile, []); + const [displayStatus, setDisplayStatus] = useState("loading"); const [name, setName] = useState(""); const [email, setEmail] = useState(""); const [error, setError] = useState(null); const fileInputRef = useRef(null); - const loadProfile = useCallback(async () => { - setStatus("loading"); - setError(null); - try { - const data = await getProfile(); - setProfile(data); - setName(data.name); - setEmail(data.email); - setStatus("ready"); - } catch { - setProfile(null); - setStatus("error"); + // Sync loaded profile into form fields + useEffect(() => { + if (profile) { + setName(profile.name); + setEmail(profile.email); + setDisplayStatus("ready"); + setError(null); } - }, []); + }, [profile]); useEffect(() => { - void loadProfile(); - }, [loadProfile]); + if (loadStatus === "error") { + setDisplayStatus("error"); + } + }, [loadStatus]); const handleSave = useCallback(async () => { if (!name.trim()) { @@ -45,16 +43,15 @@ export const ProfilePage = () => { return; } - setStatus("saving"); + setDisplayStatus("saving"); setError(null); try { - const updated = await updateProfile({ name: name.trim(), email: email.trim() }); - setProfile(updated); + await updateProfile({ name: name.trim(), email: email.trim() }); await refreshSession(); - setStatus("ready"); + setDisplayStatus("ready"); } catch { setError("Failed to update profile"); - setStatus("ready"); + setDisplayStatus("ready"); } }, [name, email, refreshSession]); @@ -73,19 +70,19 @@ export const ProfilePage = () => { return; } - setStatus("saving"); + setDisplayStatus("saving"); setError(null); try { - const updated = await uploadAvatar(file); - setProfile(updated); + await uploadAvatar(file); await refreshSession(); - setStatus("ready"); + reload(); + setDisplayStatus("ready"); } catch { setError("Failed to upload avatar"); - setStatus("ready"); + setDisplayStatus("ready"); } }, - [refreshSession] + [refreshSession, reload] ); const avatarUrl = profile?.avatar_url ?? null; @@ -94,19 +91,19 @@ export const ProfilePage = () => {

Profile

- {status === "loading" &&

Loading profile...

} + {displayStatus === "loading" &&

Loading profile...

} - {status === "error" && ( + {displayStatus === "error" && (

Failed to load profile

-
)} - {(status === "ready" || status === "saving") && profile && ( + {(displayStatus === "ready" || displayStatus === "saving") && profile && (
@@ -118,11 +115,11 @@ export const ProfilePage = () => {
@@ -130,9 +115,9 @@ export const ProjectsPage = () => { {isEmpty &&

No projects yet. Create your first project above.

} - {status === "ready" && projects.length > 0 && ( + {status === "ready" && safeProjects.length > 0 && (
- {projects.map((project) => ( + {safeProjects.map((project) => (

{project.name}

diff --git a/apps/web/src/pages/settings.tsx b/apps/web/src/pages/settings.tsx index 30d53d0..2aa1756 100644 --- a/apps/web/src/pages/settings.tsx +++ b/apps/web/src/pages/settings.tsx @@ -1,10 +1,9 @@ -import { useCallback, useEffect, useState } from "react"; +import { useEffect, useState } from "react"; import { Link, Outlet, useLocation, useOutletContext } from "react-router-dom"; import { getUserConfig, updateUserConfig, type UserConfig, type UserConfigUpdate } from "../api/settings"; import { Icon } from "../components/icon"; - -type SettingsStatus = "loading" | "ready" | "error"; +import { useAsyncData } from "../hooks/use-async-data"; const TABS = [ { label: "General", path: "general" }, @@ -26,7 +25,7 @@ type SettingsOutletContext = { export const SettingsPage = () => { const location = useLocation(); - const [status, setStatus] = useState("loading"); + const { data: loadedConfig, status, reload } = useAsyncData(getUserConfig, []); const [config, setConfig] = useState({ theme: "system", default_editor: null, @@ -36,19 +35,12 @@ export const SettingsPage = () => { }); const [saveStatus, setSaveStatus] = useState<"idle" | "saving" | "saved" | "error">("idle"); - const loadConfig = useCallback(async () => { - try { - const data = await getUserConfig(); - setConfig(data); - setStatus("ready"); - } catch { - setStatus("error"); - } - }, []); - + // Sync loaded config into local editable state useEffect(() => { - void loadConfig(); - }, [loadConfig]); + if (loadedConfig) { + setConfig(loadedConfig); + } + }, [loadedConfig]); const handleChange = (key: keyof UserConfigUpdate, value: string | null) => { setConfig((prev) => ({ ...prev, [key]: value })); @@ -86,7 +78,7 @@ export const SettingsPage = () => { return (

Failed to load settings

- diff --git a/apps/web/src/pages/ssh-keys.tsx b/apps/web/src/pages/ssh-keys.tsx index 60fc57f..e1a5d0f 100644 --- a/apps/web/src/pages/ssh-keys.tsx +++ b/apps/web/src/pages/ssh-keys.tsx @@ -1,13 +1,12 @@ -import { useEffect, useState } from "react"; +import { useState } from "react"; import { useNavigate } from "react-router-dom"; import { createSSHKey, deleteSSHKey, listSSHKeys, signPayload, verifySignature, type SSHKey } from "../api/ssh_keys"; import { Icon } from "../components/icon"; +import { useAsyncData } from "../hooks/use-async-data"; export const SSHKeysPage = () => { const navigate = useNavigate(); - const [keys, setKeys] = useState([]); - const [loading, setLoading] = useState(true); - const [error, setError] = useState(null); + const { data: keys, status, error, reload: loadKeys } = useAsyncData(listSSHKeys, []); const [newKeyName, setNewKeyName] = useState(""); const [generating, setGenerating] = useState(false); const [signPayloads, setSignPayloads] = useState>({}); @@ -17,23 +16,9 @@ export const SSHKeysPage = () => { const [verifySignatures, setVerifySignatures] = useState>({}); const [verifyResults, setVerifyResults] = useState>({}); const [verifying, setVerifying] = useState>({}); + const [mutationError, setMutationError] = useState(null); - useEffect(() => { - loadKeys(); - }, []); - - async function loadKeys() { - try { - setLoading(true); - const data = await listSSHKeys(); - setKeys(data); - setError(null); - } catch { - setError("Failed to load SSH keys"); - } finally { - setLoading(false); - } - } + const safeKeys = keys ?? []; async function handleGenerate(e: React.FormEvent) { e.preventDefault(); @@ -45,7 +30,7 @@ export const SSHKeysPage = () => { setNewKeyName(""); await loadKeys(); } catch { - setError("Failed to generate SSH key"); + setMutationError("Failed to generate SSH key"); } finally { setGenerating(false); } @@ -58,7 +43,7 @@ export const SSHKeysPage = () => { await deleteSSHKey(keyId); await loadKeys(); } catch { - setError("Failed to delete SSH key"); + setMutationError("Failed to delete SSH key"); } } @@ -74,9 +59,9 @@ export const SSHKeysPage = () => { setSigning((prev) => ({ ...prev, [keyId]: true })); const result = await signPayload(keyId, { payload: payload.trim() }); setSignatures((prev) => ({ ...prev, [keyId]: result.signature })); - setError(null); + setMutationError(null); } catch { - setError("Failed to sign payload"); + setMutationError("Failed to sign payload"); } finally { setSigning((prev) => ({ ...prev, [keyId]: false })); } @@ -94,15 +79,15 @@ export const SSHKeysPage = () => { signature: signature.trim(), }); setVerifyResults((prev) => ({ ...prev, [keyId]: result.valid })); - setError(null); + setMutationError(null); } catch { - setError("Failed to verify signature"); + setMutationError("Failed to verify signature"); } finally { setVerifying((prev) => ({ ...prev, [keyId]: false })); } } - if (loading) return
Loading...
; + if (status === "loading") return
Loading...
; return (
@@ -116,7 +101,7 @@ export const SSHKeysPage = () => {
- {error &&
{error}
} + {mutationError &&
{mutationError}
}
@@ -145,11 +130,21 @@ export const SSHKeysPage = () => { + {status === "error" && ( +
+

Failed to load SSH keys

+ +
+ )} +
- {keys.length === 0 ? ( + {safeKeys.length === 0 ? (

No SSH keys yet. Generate one above.

) : ( - keys.map((key) => ( + safeKeys.map((key) => (

{key.name}

diff --git a/apps/web/src/pages/tool-workshop.tsx b/apps/web/src/pages/tool-workshop.tsx index a1550e5..814003b 100644 --- a/apps/web/src/pages/tool-workshop.tsx +++ b/apps/web/src/pages/tool-workshop.tsx @@ -1,6 +1,7 @@ import { useCallback, useEffect, useState } from "react"; import { Icon } from "../components/icon"; import { useMobileViewport } from "../hooks/use-mobile-viewport"; +import { extractErrorMessage } from "../utils/errors"; import { MobileListView } from "../components/mobile-list-view"; import { MobileDetailView } from "../components/mobile-detail-view"; import { MobileEditView } from "../components/mobile-edit-view"; @@ -201,16 +202,6 @@ export const ToolWorkshopPage = () => { setShowFolderForm(false); }; - const extractErrorMessage = (err: unknown): string => { - const axiosError = err as { response?: { data?: { detail?: string | Array<{msg?: string}> } } }; - const detail = axiosError?.response?.data?.detail; - if (typeof detail === 'string') return detail; - if (Array.isArray(detail)) { - return detail.map(d => typeof d === 'string' ? d : d.msg || JSON.stringify(d)).join(', '); - } - return "Failed to save"; - }; - const handleToolTypeSubmit = async (e: React.FormEvent) => { e.preventDefault(); setToolTypeError(null); diff --git a/apps/web/src/utils/errors.ts b/apps/web/src/utils/errors.ts new file mode 100644 index 0000000..9d81f95 --- /dev/null +++ b/apps/web/src/utils/errors.ts @@ -0,0 +1,9 @@ +export const extractErrorMessage = (err: unknown): string => { + const axiosError = err as { response?: { data?: { detail?: string | Array<{ msg?: string }> } } }; + const detail = axiosError?.response?.data?.detail; + if (typeof detail === "string") return detail; + if (Array.isArray(detail)) { + return detail.map((d) => typeof d === "string" ? d : d.msg || JSON.stringify(d)).join(", "); + } + return "Failed to save"; +};