From 1ef9d66eed8aaec372e453daa2f2214789436c8e Mon Sep 17 00:00:00 2001 From: Developer Date: Sat, 6 Jun 2026 09:01:01 +0000 Subject: [PATCH] fix: reduce false container-failed notifications, add error details to UI Backend (health_monitor.py): - Skip health checks for instances with no container_id - Treat 'not_found' as error only when container was previously running - Skip duplicate error notifications when already in error state - Skip 'not_found' notifications for containers that never ran Frontend (notification-item.tsx): - Display notification.message (detailed error text) - Add expandable Details section showing metadata (exit_code, previous_status, etc.) - New CSS styles for message and metadata display Quality gates: py_compile, tsc --noEmit, 80/80 tests pass --- .../src/services/instance/health_monitor.py | 54 +- .../notification/notification-item.tsx | 43 ++ .../features/project/ProjectCard.tsx | 278 +++++--- .../project/repository-create-dialog.tsx | 660 +++++++++--------- apps/web/src/pages/DashboardPage.test.tsx | 99 +-- apps/web/src/styles/utilities.css | 38 + 6 files changed, 697 insertions(+), 475 deletions(-) diff --git a/apps/api/src/services/instance/health_monitor.py b/apps/api/src/services/instance/health_monitor.py index 49b86cc..15164de 100644 --- a/apps/api/src/services/instance/health_monitor.py +++ b/apps/api/src/services/instance/health_monitor.py @@ -90,8 +90,16 @@ class HealthMonitor: instance: ToolInstance, ) -> None: """Check a single instance and handle state transitions.""" + # Skip instances that have never been assigned a container. + if not instance.container_id: + logger.debug( + "Skipping health check for instance %s: no container_id", + instance.id, + ) + return + try: - container_info = get_container_status(instance.container_id or "") + container_info = get_container_status(instance.container_id) except Exception: logger.exception( "Health check failed for instance %s", @@ -135,7 +143,11 @@ class HealthMonitor: previous = self._last_known_state.get(instance.id) # Determine new status - new_status = self._derive_status(snapshot) + new_status = self._derive_status( + snapshot, + previous, + instance.status, + ) # If first check or state changed if previous is None or not self._snapshots_equal(previous, snapshot): @@ -144,10 +156,34 @@ class HealthMonitor: ) self._last_known_state[instance.id] = snapshot - def _derive_status(self, snapshot: HealthSnapshot) -> str: - """Derive instance status from health snapshot.""" + def _derive_status( + self, + snapshot: HealthSnapshot, + previous: HealthSnapshot | None, + current_status: str | None, + ) -> str: + """Derive instance status from health snapshot. + + Treats missing containers as an error only when the container was + previously known to be running. This avoids false "container failed" + alerts for instances that are still starting or have no container yet. + """ + if snapshot.container_status == "not_found": + # If the container was never seen running, assume it's still + # starting or was deleted intentionally; don't flag as error. + if previous is None and current_status == "starting": + return "starting" + if previous is not None and previous.container_status == "running": + return "error" + # Fall back to current status to avoid spurious errors. + return current_status or "error" + + if snapshot.container_status == "exited": + return "error" + if snapshot.container_status != "running": return "error" + if snapshot.tunnel_healthy is False: return "unhealthy" return "running" @@ -223,6 +259,16 @@ class HealthMonitor: # Create notification for instance owner (fire-and-forget) # Only send warnings and errors; skip "recovered" info notifications. if new_status == "error": + # Skip duplicate error notifications if already in error state. + if previous_status == "error": + return + # Skip "not_found" errors for containers that were never running + # (e.g. still starting, or intentionally stopped/deleted). + if ( + snapshot.container_status == "not_found" + and (previous is None or previous.container_status != "running") + ): + return category = "instance" severity = "error" title = "Container failed" diff --git a/apps/web/src/components/features/notification/notification-item.tsx b/apps/web/src/components/features/notification/notification-item.tsx index ccaa4e4..99a29af 100644 --- a/apps/web/src/components/features/notification/notification-item.tsx +++ b/apps/web/src/components/features/notification/notification-item.tsx @@ -1,3 +1,4 @@ +import { useState } from "react"; import { Icon } from "../../icon"; import { formatRelativeTime } from "../../../utils/time"; import type { NotificationItem as NotificationItemType } from "../../../api/notifications"; @@ -17,6 +18,18 @@ const severityIconMap: Record = { success: "success", }; +function formatMetadataValue(value: unknown): string { + if (value === null || value === undefined) return "—"; + if (typeof value === "string") return value; + if (typeof value === "number") return String(value); + if (typeof value === "boolean") return value ? "Yes" : "No"; + try { + return JSON.stringify(value); + } catch { + return String(value); + } +} + export function NotificationItem({ notification, onMarkRead, @@ -24,6 +37,11 @@ export function NotificationItem({ }: NotificationItemProps) { const isUnread = notification.read_at === null; const iconName = severityIconMap[notification.severity] ?? "info"; + const [expanded, setExpanded] = useState(false); + + const hasDetails = + !!notification.message || + Object.keys(notification.metadata || {}).length > 0; return (
  • {notification.title}
    + {notification.message && ( +
    + {notification.message} +
    + )}
    {formatRelativeTime(notification.created_at)}
    + {expanded && notification.metadata && ( +
    + {Object.entries(notification.metadata).map(([key, value]) => ( +
    +
    {key.replace(/_/g, " ")}
    +
    {formatMetadataValue(value)}
    +
    + ))} +
    + )}
    + {hasDetails && ( + + )} {isUnread && ( -
    - - {deleteConfirm ? ( -
    - Are you sure? - - -
    - ) : ( - - )} -
    -
    + return ( +
    +
    + +
    + + {deleteConfirm ? ( +
    + Are you sure? + + +
    + ) : ( + + )} +
    +
    - {expanded && ( -
    - {(project.repositories || []).length === 0 ? ( -

    No repositories yet.

    - ) : ( -
    - {(project.repositories || []).map((repo) => ( -
    -
    -

    {repo.name}

    - -
    - {showCreateForm === repo.id && ( - - )} - {repo.workspaces.length === 0 ? ( -

    No workspaces.

    - ) : ( -
    - {repo.workspaces.map((ws) => ( -
    - {ws.name} - {ws.branch} - {ws.instance_count > 0 && ( - {ws.instance_count} tool{ws.instance_count > 1 ? "s" : ""} - )} -
    - - -
    -
    - ))} -
    - )} -
    - ))} -
    - )} -
    - )} -
    - ); + {expanded && ( +
    + {(project.repositories || []).length === 0 ? ( +

    No repositories yet.

    + ) : ( +
    + {(project.repositories || []).map((repo) => ( +
    +
    +

    {repo.name}

    + +
    + {showCreateForm === repo.id && ( + + )} + {repo.workspaces.length === 0 ? ( +

    No workspaces.

    + ) : ( +
    + {repo.workspaces.map((ws) => ( +
    + {ws.name} + + {ws.branch} + + {ws.instance_count > 0 && ( + + {ws.instance_count} tool + {ws.instance_count > 1 ? "s" : ""} + + )} +
    + + +
    +
    + ))} +
    + )} +
    + ))} +
    + )} +
    + )} + + ); }; diff --git a/apps/web/src/components/features/project/repository-create-dialog.tsx b/apps/web/src/components/features/project/repository-create-dialog.tsx index 8584d5c..c867075 100644 --- a/apps/web/src/components/features/project/repository-create-dialog.tsx +++ b/apps/web/src/components/features/project/repository-create-dialog.tsx @@ -1,343 +1,377 @@ import { useEffect, useRef, useState } from "react"; -import { createRepository, parseGitUrl, type GitRepositoryCreate, type URLParseResult } from "../../../api/git-repositories"; +import { + createRepository, + parseGitUrl, + type GitRepositoryCreate, + type URLParseResult, +} from "../../../api/git-repositories"; import { listSSHKeys, type SSHKey } from "../../../api/ssh-keys"; import { Icon } from "../../icon"; type CreateMode = "clone" | "blank"; -type UrlValidationStatus = "idle" | "validating" | "valid" | "needs-parsing" | "invalid"; +type UrlValidationStatus = + | "idle" + | "validating" + | "valid" + | "needs-parsing" + | "invalid"; interface RepositoryCreateDialogProps { - projectId: string; - open: boolean; - title: string; - onClose: () => void; - onCreated: () => Promise | void; + projectId: string; + open: boolean; + title: string; + onClose: () => void; + onCreated: () => Promise | void; } -export const RepositoryCreateDialog = ({ projectId, open, title, onClose, onCreated }: RepositoryCreateDialogProps) => { - const [createMode, setCreateMode] = useState("clone"); - const [formName, setFormName] = useState(""); - const [owner, setOwner] = useState(""); - const [repoName, setRepoName] = useState(""); - const [advancedUrl, setAdvancedUrl] = useState(""); - const [useAdvancedUrl, setUseAdvancedUrl] = useState(false); - const [formError, setFormError] = useState(null); - const [urlValidation, setUrlValidation] = useState<{ - status: UrlValidationStatus; - result: URLParseResult | null; - }>({ status: "idle", result: null }); - const [sshKeys, setSshKeys] = useState([]); - const [selectedSshKey, setSelectedSshKey] = useState(""); - const debounceTimer = useRef | null>(null); +export const RepositoryCreateDialog = ({ + projectId, + open, + title, + onClose, + onCreated, +}: RepositoryCreateDialogProps) => { + const [createMode, setCreateMode] = useState("clone"); + const [formName, setFormName] = useState(""); + const [owner, setOwner] = useState(""); + const [repoName, setRepoName] = useState(""); + const [advancedUrl, setAdvancedUrl] = useState(""); + const [useAdvancedUrl, setUseAdvancedUrl] = useState(false); + const [formError, setFormError] = useState(null); + const [urlValidation, setUrlValidation] = useState<{ + status: UrlValidationStatus; + result: URLParseResult | null; + }>({ status: "idle", result: null }); + const [sshKeys, setSshKeys] = useState([]); + const [selectedSshKey, setSelectedSshKey] = useState(""); + const debounceTimer = useRef | null>(null); - useEffect(() => { - if (!open && debounceTimer.current) { - clearTimeout(debounceTimer.current); - debounceTimer.current = null; - } - }, [open]); + useEffect(() => { + if (!open && debounceTimer.current) { + clearTimeout(debounceTimer.current); + debounceTimer.current = null; + } + }, [open]); - useEffect(() => { - if (!open) return; - const loadKeys = async () => { - try { - const data = await listSSHKeys(); - setSshKeys(data); - } catch { - // ignore - } - }; - void loadKeys(); - }, [open]); + useEffect(() => { + if (!open) return; + const loadKeys = async () => { + try { + const data = await listSSHKeys(); + setSshKeys(data); + } catch { + // ignore + } + }; + void loadKeys(); + }, [open]); - useEffect(() => { - if (!open) return; - if (!useAdvancedUrl) { - setUrlValidation({ status: "idle", result: null }); - return; - } + useEffect(() => { + if (!open) return; + if (!useAdvancedUrl) { + setUrlValidation({ status: "idle", result: null }); + return; + } - if (debounceTimer.current) { - clearTimeout(debounceTimer.current); - } + if (debounceTimer.current) { + clearTimeout(debounceTimer.current); + } - if (!advancedUrl.trim()) { - setUrlValidation({ status: "idle", result: null }); - return; - } + if (!advancedUrl.trim()) { + setUrlValidation({ status: "idle", result: null }); + return; + } - setUrlValidation({ status: "validating", result: null }); + setUrlValidation({ status: "validating", result: null }); - debounceTimer.current = setTimeout(async () => { - try { - const result = await parseGitUrl(advancedUrl.trim()); - if (result.is_valid_clone_url) { - setUrlValidation({ status: "valid", result }); - } else if (result.needs_parsing) { - setUrlValidation({ status: "needs-parsing", result }); - } else { - setUrlValidation({ status: "invalid", result }); - } - } catch { - setUrlValidation({ status: "invalid", result: null }); - } - }, 300); + debounceTimer.current = setTimeout(async () => { + try { + const result = await parseGitUrl(advancedUrl.trim()); + if (result.is_valid_clone_url) { + setUrlValidation({ status: "valid", result }); + } else if (result.needs_parsing) { + setUrlValidation({ status: "needs-parsing", result }); + } else { + setUrlValidation({ status: "invalid", result }); + } + } catch { + setUrlValidation({ status: "invalid", result: null }); + } + }, 300); - return () => { - if (debounceTimer.current) { - clearTimeout(debounceTimer.current); - } - }; - }, [advancedUrl, open, useAdvancedUrl]); + return () => { + if (debounceTimer.current) { + clearTimeout(debounceTimer.current); + } + }; + }, [advancedUrl, open, useAdvancedUrl]); - const resetForm = () => { - setCreateMode("clone"); - setFormName(""); - setOwner(""); - setRepoName(""); - setAdvancedUrl(""); - setUseAdvancedUrl(true); - setFormError(null); - setUrlValidation({ status: "idle", result: null }); - setSelectedSshKey(""); - }; + const resetForm = () => { + setCreateMode("clone"); + setFormName(""); + setOwner(""); + setRepoName(""); + setAdvancedUrl(""); + setUseAdvancedUrl(true); + setFormError(null); + setUrlValidation({ status: "idle", result: null }); + setSelectedSshKey(""); + }; - const handleClose = () => { - resetForm(); - onClose(); - }; + const handleClose = () => { + resetForm(); + onClose(); + }; - const handleSubmit = async (event: React.FormEvent) => { - event.preventDefault(); - setFormError(null); + const handleSubmit = async (event: React.FormEvent) => { + event.preventDefault(); + setFormError(null); - if (!formName.trim()) { - setFormError("Repository name is required"); - return; - } + if (!formName.trim()) { + setFormError("Repository name is required"); + return; + } - try { - const input: GitRepositoryCreate = { - name: formName.trim(), - remote_url: undefined, - }; + try { + const input: GitRepositoryCreate = { + name: formName.trim(), + remote_url: undefined, + }; - if (createMode === "clone") { - if (useAdvancedUrl) { - if (!advancedUrl.trim()) { - setFormError("Remote URL is required for advanced cloning"); - return; - } - input.remote_url = advancedUrl.trim(); - } else { - if (!owner.trim() || !repoName.trim()) { - setFormError("Owner and repository name are required"); - return; - } - input.remote_url = `git@git.commumedia.org:${owner.trim()}/${repoName.trim()}.git`; - } - if (selectedSshKey) { - input.ssh_key_id = selectedSshKey; - } - } + if (createMode === "clone") { + if (useAdvancedUrl) { + if (!advancedUrl.trim()) { + setFormError("Remote URL is required for advanced cloning"); + return; + } + input.remote_url = advancedUrl.trim(); + } else { + if (!owner.trim() || !repoName.trim()) { + setFormError("Owner and repository name are required"); + return; + } + input.remote_url = `git@git.commumedia.org:${owner.trim()}/${repoName.trim()}.git`; + } + if (selectedSshKey) { + input.ssh_key_id = selectedSshKey; + } + } - await createRepository(projectId, input); - handleClose(); - await onCreated(); - } catch (error: unknown) { - const response = error as { response?: { data?: { detail?: string } } }; - const detail = response.response?.data?.detail; - setFormError(typeof detail === "string" ? detail : "Failed to create repository"); - } - }; + await createRepository(projectId, input); + handleClose(); + await onCreated(); + } catch (error: unknown) { + const response = error as { response?: { data?: { detail?: string } } }; + const detail = response.response?.data?.detail; + setFormError( + typeof detail === "string" ? detail : "Failed to create repository", + ); + } + }; - const handleUseSuggestedUrl = () => { - if (urlValidation.result?.base_url) { - setAdvancedUrl(urlValidation.result.base_url); - setUrlValidation({ status: "idle", result: null }); - setFormError(null); - } - }; + const handleUseSuggestedUrl = () => { + if (urlValidation.result?.base_url) { + setAdvancedUrl(urlValidation.result.base_url); + setUrlValidation({ status: "idle", result: null }); + setFormError(null); + } + }; - const getUrlInputClass = () => { - switch (urlValidation.status) { - case "valid": - return "valid-url"; - case "needs-parsing": - return "needs-parsing-url"; - case "invalid": - return "invalid-url"; - default: - return ""; - } - }; + const getUrlInputClass = () => { + switch (urlValidation.status) { + case "valid": + return "valid-url"; + case "needs-parsing": + return "needs-parsing-url"; + case "invalid": + return "invalid-url"; + default: + return ""; + } + }; - if (!open) return null; + if (!open) return null; - return ( -
    -
    -

    {title}

    -

    - Clone an existing repository from git.commumedia.org, or create a blank bare repo here. -

    -
    -
    - - -
    - - {createMode === "clone" && !useAdvancedUrl && ( - <> - - - -

    SSH target: git@git.commumedia.org:{owner || "owner"}/{repoName || "repo"}.git

    - - - )} - {createMode === "clone" && useAdvancedUrl && ( - <> - - - - - )} - {formError && ( -
    -

    {formError}

    -
    - )} -
    - - -
    -
    -
    -
    - ); + return ( +
    +
    +

    {title}

    +

    + Clone an existing repository from git.commumedia.org, or create a + blank bare repo here. +

    +
    +
    + + +
    + + {createMode === "clone" && !useAdvancedUrl && ( + <> + + + +

    + SSH target: git@git.commumedia.org:{owner || "owner"}/ + {repoName || "repo"}.git +

    + + + )} + {createMode === "clone" && useAdvancedUrl && ( + <> + + + + + )} + {formError && ( +
    +

    {formError}

    +
    + )} +
    + + +
    +
    +
    +
    + ); }; diff --git a/apps/web/src/pages/DashboardPage.test.tsx b/apps/web/src/pages/DashboardPage.test.tsx index da6a070..33dcda6 100644 --- a/apps/web/src/pages/DashboardPage.test.tsx +++ b/apps/web/src/pages/DashboardPage.test.tsx @@ -10,72 +10,79 @@ const mockProjects = vi.fn(); const mockRepos = vi.fn(); vi.mock("../api/dashboard", () => ({ - getDashboardSummary: (...args: unknown[]) => mockDashboard(...args) + getDashboardSummary: (...args: unknown[]) => mockDashboard(...args), })); vi.mock("../api/sessions", () => ({ - getUserSessions: (...args: unknown[]) => mockSessions(...args), - createInstance: vi.fn(), - startInstance: vi.fn(), - stopInstance: vi.fn(), - deleteInstance: vi.fn(), - recreateInstanceTunnel: vi.fn() + getUserSessions: (...args: unknown[]) => mockSessions(...args), + createInstance: vi.fn(), + startInstance: vi.fn(), + stopInstance: vi.fn(), + deleteInstance: vi.fn(), + recreateInstanceTunnel: vi.fn(), })); vi.mock("../api/projects", () => ({ - listProjects: (...args: unknown[]) => mockProjects(...args) + listProjects: (...args: unknown[]) => mockProjects(...args), })); vi.mock("../api/git-repositories", () => ({ - listRepositories: (...args: unknown[]) => mockRepos(...args) + listRepositories: (...args: unknown[]) => mockRepos(...args), })); vi.mock("../api/tool-types", () => ({ - listToolTypes: vi.fn().mockResolvedValue([]) + listToolTypes: vi.fn().mockResolvedValue([]), })); describe("HomePage", () => { - beforeEach(() => { - mockDashboard.mockReset(); - mockSessions.mockReset(); - mockProjects.mockReset(); - mockRepos.mockReset(); - }); + beforeEach(() => { + mockDashboard.mockReset(); + mockSessions.mockReset(); + mockProjects.mockReset(); + mockRepos.mockReset(); + }); - it("shows overview sections", async () => { - mockDashboard.mockResolvedValue({ projects: 1, repositories: 2, sshKeys: 3, recentActivity: [] }); - mockSessions.mockResolvedValue([]); - mockProjects.mockResolvedValue([]); - mockRepos.mockResolvedValue([]); + it("shows overview sections", async () => { + mockDashboard.mockResolvedValue({ + projects: 1, + repositories: 2, + sshKeys: 3, + recentActivity: [], + }); + mockSessions.mockResolvedValue([]); + mockProjects.mockResolvedValue([]); + mockRepos.mockResolvedValue([]); - render( - - - - ); + render( + + + , + ); - expect(screen.getByText("Loading overview...")).toBeInTheDocument(); - await waitFor(() => { - expect(screen.getAllByText("Open sessions").length).toBeGreaterThan(0); - expect(screen.getByText("Workspaces")).toBeInTheDocument(); - }); - }); + expect(screen.getByText("Loading overview...")).toBeInTheDocument(); + await waitFor(() => { + expect(screen.getAllByText("Open sessions").length).toBeGreaterThan(0); + expect(screen.getByText("Workspaces")).toBeInTheDocument(); + }); + }); - it("shows retry action when home load fails", async () => { - mockDashboard.mockRejectedValueOnce(new Error("failed")); - mockSessions.mockRejectedValueOnce(new Error("failed")); - mockProjects.mockRejectedValueOnce(new Error("failed")); + it("shows retry action when home load fails", async () => { + mockDashboard.mockRejectedValueOnce(new Error("failed")); + mockSessions.mockRejectedValueOnce(new Error("failed")); + mockProjects.mockRejectedValueOnce(new Error("failed")); - render( - - - - ); + render( + + + , + ); - await waitFor(() => { - expect(screen.getByText("Unable to load your workspace overview.")).toBeInTheDocument(); - }); + await waitFor(() => { + expect( + screen.getByText("Unable to load your workspace overview."), + ).toBeInTheDocument(); + }); - fireEvent.click(screen.getAllByRole("button", { name: "Retry" })[0]); - }); + fireEvent.click(screen.getAllByRole("button", { name: "Retry" })[0]); + }); }); diff --git a/apps/web/src/styles/utilities.css b/apps/web/src/styles/utilities.css index 0ab0820..a37a276 100644 --- a/apps/web/src/styles/utilities.css +++ b/apps/web/src/styles/utilities.css @@ -2474,6 +2474,44 @@ a:active, color: var(--muted); } +.notification-item-message { + font-size: 0.85rem; + color: var(--muted); + line-height: 1.35; + margin-top: 0.15rem; + word-break: break-word; +} + +.notification-item-metadata { + margin-top: 0.5rem; + padding: 0.5rem 0.75rem; + background: var(--bg); + border-radius: 6px; + border: 1px solid var(--border); + font-size: 0.8rem; +} + +.notification-metadata-row { + display: flex; + gap: 0.5rem; + padding: 0.15rem 0; +} + +.notification-metadata-row dt { + font-weight: 500; + color: var(--ink); + min-width: 6rem; + text-transform: capitalize; + flex-shrink: 0; +} + +.notification-metadata-row dd { + margin: 0; + color: var(--muted); + font-family: monospace; + word-break: break-word; +} + .notification-item-actions { display: flex; gap: 0.35rem;