fix: stop manifest editor base image selection loop
The ManifestEditor had a feedback loop: 1. State change → buildManifest changes → onChange notifies parent 2. Parent updates manifestData → new manifest prop 3. Loading effect sets all state from manifest (arrays get new refs even if same content) 4. New array refs → buildManifest changes → onChange fires again → loop Fix: track the last-sent manifest via a ref and only call onChange when the serialized built manifest actually differs. This breaks the cycle because after the loading effect syncs state, the rebuilt manifest is identical in content so we skip the parent notification.
This commit is contained in:
@@ -50,7 +50,9 @@ class ToolTypeCreate(BaseModel):
|
|||||||
@classmethod
|
@classmethod
|
||||||
def validate_definition_type(cls, v: str) -> str:
|
def validate_definition_type(cls, v: str) -> str:
|
||||||
if v not in ("compose", "dockerfile", "manifest"):
|
if v not in ("compose", "dockerfile", "manifest"):
|
||||||
raise ValueError("definition_type must be 'compose', 'dockerfile', or 'manifest'")
|
raise ValueError(
|
||||||
|
"definition_type must be 'compose', 'dockerfile', or 'manifest'"
|
||||||
|
)
|
||||||
return v
|
return v
|
||||||
|
|
||||||
@field_validator("compose_template")
|
@field_validator("compose_template")
|
||||||
@@ -61,7 +63,9 @@ class ToolTypeCreate(BaseModel):
|
|||||||
return v
|
return v
|
||||||
|
|
||||||
if v is None or not v.strip():
|
if v is None or not v.strip():
|
||||||
raise ValueError("compose_template is required when definition_type is 'compose'")
|
raise ValueError(
|
||||||
|
"compose_template is required when definition_type is 'compose'"
|
||||||
|
)
|
||||||
|
|
||||||
validate_compose_yaml(v)
|
validate_compose_yaml(v)
|
||||||
return v
|
return v
|
||||||
@@ -74,7 +78,9 @@ class ToolTypeCreate(BaseModel):
|
|||||||
return v
|
return v
|
||||||
|
|
||||||
if v is None or not v.strip():
|
if v is None or not v.strip():
|
||||||
raise ValueError("dockerfile_template is required when definition_type is 'dockerfile'")
|
raise ValueError(
|
||||||
|
"dockerfile_template is required when definition_type is 'dockerfile'"
|
||||||
|
)
|
||||||
|
|
||||||
if not v.strip().startswith("FROM"):
|
if not v.strip().startswith("FROM"):
|
||||||
raise ValueError("Dockerfile must start with a FROM instruction")
|
raise ValueError("Dockerfile must start with a FROM instruction")
|
||||||
@@ -116,7 +122,9 @@ class ToolTypeCreate(BaseModel):
|
|||||||
for var in v:
|
for var in v:
|
||||||
placeholder = f"{{{{{var}}}}}"
|
placeholder = f"{{{{{var}}}}}"
|
||||||
if placeholder not in template:
|
if placeholder not in template:
|
||||||
raise ValueError(f"Required variable '{var}' not found in compose template")
|
raise ValueError(
|
||||||
|
f"Required variable '{var}' not found in compose template"
|
||||||
|
)
|
||||||
|
|
||||||
return v
|
return v
|
||||||
|
|
||||||
@@ -124,23 +132,39 @@ class ToolTypeCreate(BaseModel):
|
|||||||
def validate_templates(self) -> "ToolTypeCreate":
|
def validate_templates(self) -> "ToolTypeCreate":
|
||||||
if self.definition_type == "manifest":
|
if self.definition_type == "manifest":
|
||||||
if self.manifest_id is None:
|
if self.manifest_id is None:
|
||||||
raise ValueError("manifest_id is required when definition_type is 'manifest'")
|
raise ValueError(
|
||||||
|
"manifest_id is required when definition_type is 'manifest'"
|
||||||
|
)
|
||||||
return self
|
return self
|
||||||
|
|
||||||
if self.definition_type == "dockerfile" and (self.dockerfile_template is None or not self.dockerfile_template.strip()):
|
if self.definition_type == "dockerfile" and (
|
||||||
raise ValueError("dockerfile_template is required when definition_type is 'dockerfile'")
|
self.dockerfile_template is None or not self.dockerfile_template.strip()
|
||||||
if self.definition_type == "compose" and (self.compose_template is None or not self.compose_template.strip()):
|
):
|
||||||
raise ValueError("compose_template is required when definition_type is 'compose'")
|
raise ValueError(
|
||||||
|
"dockerfile_template is required when definition_type is 'dockerfile'"
|
||||||
|
)
|
||||||
|
if self.definition_type == "compose" and (
|
||||||
|
self.compose_template is None or not self.compose_template.strip()
|
||||||
|
):
|
||||||
|
raise ValueError(
|
||||||
|
"compose_template is required when definition_type is 'compose'"
|
||||||
|
)
|
||||||
|
|
||||||
# Validate that default_port is exposed in compose template (only if requires_port)
|
# 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:
|
if (
|
||||||
|
self.requires_port
|
||||||
|
and self.definition_type == "compose"
|
||||||
|
and self.compose_template
|
||||||
|
):
|
||||||
try:
|
try:
|
||||||
parsed = validate_compose_yaml(self.compose_template)
|
parsed = validate_compose_yaml(self.compose_template)
|
||||||
except ValueError:
|
except ValueError:
|
||||||
return self
|
return self
|
||||||
|
|
||||||
if not check_port_exposed(parsed, self.default_port):
|
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.")
|
raise ValueError(
|
||||||
|
f"Port {self.default_port} is not exposed in the compose template. Add it to the 'ports' section."
|
||||||
|
)
|
||||||
|
|
||||||
return self
|
return self
|
||||||
|
|
||||||
@@ -167,7 +191,9 @@ class ToolTypeUpdate(BaseModel):
|
|||||||
if v is None:
|
if v is None:
|
||||||
return v
|
return v
|
||||||
if v not in ("compose", "dockerfile", "manifest"):
|
if v not in ("compose", "dockerfile", "manifest"):
|
||||||
raise ValueError("definition_type must be 'compose', 'dockerfile', or 'manifest'")
|
raise ValueError(
|
||||||
|
"definition_type must be 'compose', 'dockerfile', or 'manifest'"
|
||||||
|
)
|
||||||
return v
|
return v
|
||||||
|
|
||||||
@field_validator("interface_type")
|
@field_validator("interface_type")
|
||||||
@@ -262,7 +288,10 @@ async def create_tool_type(
|
|||||||
# Check for duplicate name
|
# Check for duplicate name
|
||||||
existing = await session.scalar(select(ToolType).where(ToolType.name == data.name))
|
existing = await session.scalar(select(ToolType).where(ToolType.name == data.name))
|
||||||
if existing:
|
if existing:
|
||||||
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="tool type with this name already exists")
|
raise HTTPException(
|
||||||
|
status_code=status.HTTP_409_CONFLICT,
|
||||||
|
detail="tool type with this name already exists",
|
||||||
|
)
|
||||||
|
|
||||||
tool_type = ToolType(
|
tool_type = ToolType(
|
||||||
name=data.name,
|
name=data.name,
|
||||||
@@ -336,7 +365,9 @@ async def get_tool_type(
|
|||||||
await _get_user(session, user_id)
|
await _get_user(session, user_id)
|
||||||
tool_type = await session.get(ToolType, tool_type_id)
|
tool_type = await session.get(ToolType, tool_type_id)
|
||||||
if tool_type is None:
|
if tool_type is None:
|
||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="tool type not found")
|
raise HTTPException(
|
||||||
|
status_code=status.HTTP_404_NOT_FOUND, detail="tool type not found"
|
||||||
|
)
|
||||||
return tool_type
|
return tool_type
|
||||||
|
|
||||||
|
|
||||||
@@ -368,7 +399,9 @@ async def update_tool_type(
|
|||||||
|
|
||||||
tool_type = await session.get(ToolType, tool_type_id)
|
tool_type = await session.get(ToolType, tool_type_id)
|
||||||
if tool_type is None:
|
if tool_type is None:
|
||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="tool type not found")
|
raise HTTPException(
|
||||||
|
status_code=status.HTTP_404_NOT_FOUND, detail="tool type not found"
|
||||||
|
)
|
||||||
|
|
||||||
# Built-in tool types can now be modified
|
# Built-in tool types can now be modified
|
||||||
|
|
||||||
@@ -381,7 +414,7 @@ async def update_tool_type(
|
|||||||
if new_port <= 0 or new_port > 65535:
|
if new_port <= 0 or new_port > 65535:
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_400_BAD_REQUEST,
|
status_code=status.HTTP_400_BAD_REQUEST,
|
||||||
detail="Port must be between 1 and 65535"
|
detail="Port must be between 1 and 65535",
|
||||||
)
|
)
|
||||||
|
|
||||||
# Only validate port exposure for compose definitions
|
# Only validate port exposure for compose definitions
|
||||||
@@ -394,12 +427,11 @@ async def update_tool_type(
|
|||||||
if not check_port_exposed(parsed, new_port):
|
if not check_port_exposed(parsed, new_port):
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_400_BAD_REQUEST,
|
status_code=status.HTTP_400_BAD_REQUEST,
|
||||||
detail=f"Port {new_port} is not exposed in the compose template"
|
detail=f"Port {new_port} is not exposed in the compose template",
|
||||||
)
|
)
|
||||||
except ValueError as e:
|
except ValueError as e:
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_400_BAD_REQUEST,
|
status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)
|
||||||
detail=str(e)
|
|
||||||
)
|
)
|
||||||
|
|
||||||
# Validate required variables for compose definitions
|
# Validate required variables for compose definitions
|
||||||
@@ -509,7 +541,9 @@ async def validate_tool_type(
|
|||||||
await _get_user(session, user_id)
|
await _get_user(session, user_id)
|
||||||
tool_type = await session.get(ToolType, tool_type_id)
|
tool_type = await session.get(ToolType, tool_type_id)
|
||||||
if tool_type is None:
|
if tool_type is None:
|
||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="tool type not found")
|
raise HTTPException(
|
||||||
|
status_code=status.HTTP_404_NOT_FOUND, detail="tool type not found"
|
||||||
|
)
|
||||||
|
|
||||||
errors = []
|
errors = []
|
||||||
|
|
||||||
@@ -564,7 +598,9 @@ async def delete_tool_type(
|
|||||||
|
|
||||||
tool_type = await session.get(ToolType, tool_type_id)
|
tool_type = await session.get(ToolType, tool_type_id)
|
||||||
if tool_type is None:
|
if tool_type is None:
|
||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="tool type not found")
|
raise HTTPException(
|
||||||
|
status_code=status.HTTP_404_NOT_FOUND, detail="tool type not found"
|
||||||
|
)
|
||||||
|
|
||||||
# Built-in tool types can now be deleted
|
# Built-in tool types can now be deleted
|
||||||
|
|
||||||
|
|||||||
@@ -1,4 +1,4 @@
|
|||||||
import { useState, useEffect, useCallback } from "react";
|
import { useState, useEffect, useCallback, useRef } from "react";
|
||||||
import { Icon } from "./icon";
|
import { Icon } from "./icon";
|
||||||
import { extractErrorMessage } from "../utils/errors";
|
import { extractErrorMessage } from "../utils/errors";
|
||||||
import {
|
import {
|
||||||
@@ -170,11 +170,20 @@ export const ManifestEditor = ({
|
|||||||
baseDefinitionId,
|
baseDefinitionId,
|
||||||
]);
|
]);
|
||||||
|
|
||||||
// Notify parent of changes
|
// Notify parent of changes — only when built manifest actually differs
|
||||||
|
// from what we last sent, to avoid feedback loops with the manifest prop.
|
||||||
|
const lastSentRef = useRef<string>("");
|
||||||
|
const onChangeRef = useRef(onChange);
|
||||||
|
onChangeRef.current = onChange;
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
const m = buildManifest();
|
const m = buildManifest();
|
||||||
onChange(m);
|
const serialized = JSON.stringify(m);
|
||||||
}, [buildManifest, onChange]);
|
if (serialized !== lastSentRef.current) {
|
||||||
|
lastSentRef.current = serialized;
|
||||||
|
onChangeRef.current(m);
|
||||||
|
}
|
||||||
|
}, [buildManifest]);
|
||||||
|
|
||||||
const handlePreview = async () => {
|
const handlePreview = async () => {
|
||||||
if (!definitionId) {
|
if (!definitionId) {
|
||||||
|
|||||||
@@ -1,8 +1,5 @@
|
|||||||
import { useCallback, useEffect, useState } from "react";
|
import { useCallback, useEffect, useState } from "react";
|
||||||
import {
|
import { ErrorState, LoadingState } from "../components/data-states";
|
||||||
ErrorState,
|
|
||||||
LoadingState,
|
|
||||||
} from "../components/data-states";
|
|
||||||
import { Icon } from "../components/icon";
|
import { Icon } from "../components/icon";
|
||||||
import { useMobileViewport } from "../hooks/use-mobile-viewport";
|
import { useMobileViewport } from "../hooks/use-mobile-viewport";
|
||||||
import { extractErrorMessage } from "../utils/errors";
|
import { extractErrorMessage } from "../utils/errors";
|
||||||
@@ -75,7 +72,6 @@ export const ToolWorkshopPage = () => {
|
|||||||
const [toolTypeError, setToolTypeError] = useState<string | null>(null);
|
const [toolTypeError, setToolTypeError] = useState<string | null>(null);
|
||||||
const [toolTypeDirty, setToolTypeDirty] = useState(false);
|
const [toolTypeDirty, setToolTypeDirty] = useState(false);
|
||||||
|
|
||||||
|
|
||||||
const selectedToolType =
|
const selectedToolType =
|
||||||
(toolTypes || []).find((t) => t.id === selectedToolTypeId) || null;
|
(toolTypes || []).find((t) => t.id === selectedToolTypeId) || null;
|
||||||
|
|
||||||
@@ -215,7 +211,9 @@ export const ToolWorkshopPage = () => {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
} else if (!manifestData) {
|
} else if (!manifestData) {
|
||||||
setToolTypeError("Manifest data is required for manifest definition type");
|
setToolTypeError(
|
||||||
|
"Manifest data is required for manifest definition type",
|
||||||
|
);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -300,14 +298,12 @@ export const ToolWorkshopPage = () => {
|
|||||||
description: toolTypeForm.description.trim() || undefined,
|
description: toolTypeForm.description.trim() || undefined,
|
||||||
category: toolTypeForm.category.trim() || undefined,
|
category: toolTypeForm.category.trim() || undefined,
|
||||||
interface_type: toolTypeForm.interface_type,
|
interface_type: toolTypeForm.interface_type,
|
||||||
base_image:
|
base_image: (manifestData.base_image as string) || undefined,
|
||||||
(manifestData.base_image as string) || undefined,
|
|
||||||
base_definition_id:
|
base_definition_id:
|
||||||
(manifestData.base_definition_id as string) || undefined,
|
(manifestData.base_definition_id as string) || undefined,
|
||||||
manifest: manifestData,
|
manifest: manifestData,
|
||||||
};
|
};
|
||||||
const newManifest =
|
const newManifest = await createToolDefinition(manifestPayload);
|
||||||
await createToolDefinition(manifestPayload);
|
|
||||||
manifestId = newManifest.id;
|
manifestId = newManifest.id;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -365,7 +361,6 @@ export const ToolWorkshopPage = () => {
|
|||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
||||||
if (status === "loading") {
|
if (status === "loading") {
|
||||||
return (
|
return (
|
||||||
<div className="container">
|
<div className="container">
|
||||||
@@ -935,7 +930,7 @@ export const ToolWorkshopPage = () => {
|
|||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
{(
|
{
|
||||||
<form
|
<form
|
||||||
onSubmit={handleToolTypeSubmit}
|
onSubmit={handleToolTypeSubmit}
|
||||||
className="stack"
|
className="stack"
|
||||||
@@ -1265,8 +1260,7 @@ export const ToolWorkshopPage = () => {
|
|||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
</form>
|
</form>
|
||||||
)}
|
}
|
||||||
|
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
Reference in New Issue
Block a user