diff --git a/src/routes/v2/pages/Editor/components/PinnedTaskContent/PinnedTaskContent.tsx b/src/routes/v2/pages/Editor/components/PinnedTaskContent/PinnedTaskContent.tsx index ea132bfd95..cc052f60c5 100644 --- a/src/routes/v2/pages/Editor/components/PinnedTaskContent/PinnedTaskContent.tsx +++ b/src/routes/v2/pages/Editor/components/PinnedTaskContent/PinnedTaskContent.tsx @@ -6,11 +6,10 @@ import { Icon } from "@/components/ui/icon"; import { BlockStack } from "@/components/ui/layout"; import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; import { Text } from "@/components/ui/typography"; -import { serializeComponentSpec } from "@/models/componentSpec"; +import { getTaskYamlText } from "@/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/components/actions/getTaskYamlText"; import { CodeBlock } from "@/routes/v2/shared/components/CodeBlock"; import { useSpec } from "@/routes/v2/shared/providers/SpecContext"; import { tracking } from "@/utils/tracking"; -import { componentSpecToText } from "@/utils/yaml"; interface PinnedTaskContentProps { entityId: string; @@ -35,19 +34,9 @@ export const PinnedTaskContent = observer(function PinnedTaskContent({ return ; } - const componentRef = task.componentRef; + const componentRef = task.resolvedComponentRef; const componentSpec = task.resolvedComponentSpec; - const code = (() => { - if (componentRef.text) return componentRef.text; - if (task.subgraphSpec) { - return componentSpecToText(serializeComponentSpec(task.subgraphSpec)); - } - return componentRef.spec - ? componentSpecToText( - componentRef.spec as Parameters[0], - ) - : ""; - })(); + const code = getTaskYamlText(task); return ( diff --git a/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/TaskDetails.tsx b/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/TaskDetails.tsx index 97a0457b95..a211600237 100644 --- a/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/TaskDetails.tsx +++ b/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/TaskDetails.tsx @@ -155,11 +155,7 @@ export const TaskDetails = observer(function TaskDetails({ pickDefined({ $id: task.$id, name: task.name, - componentRef: serializeComponentRef(task.componentRef), + componentRef: serializeComponentRef(task.resolvedComponentRef), arguments: task.arguments.map(serializeArgument), - isSubgraph: isGraphImplementation(task.componentRef.spec?.implementation) + isSubgraph: isGraphImplementation( + task.resolvedComponentRef.spec?.implementation, + ) ? true : undefined, }); diff --git a/src/routes/v2/shared/nodes/TaskNode/taskManifestBase.ts b/src/routes/v2/shared/nodes/TaskNode/taskManifestBase.ts index 66a2684a61..fcfc9d9484 100644 --- a/src/routes/v2/shared/nodes/TaskNode/taskManifestBase.ts +++ b/src/routes/v2/shared/nodes/TaskNode/taskManifestBase.ts @@ -46,7 +46,7 @@ export function snapshotTask( name: task.name, position: task.annotations.get(EDITOR_POSITION_ANNOTATION), data: { - componentRef: deepClone(task.componentRef), + componentRef: deepClone(task.resolvedComponentRef), isEnabled: task.isEnabled ? deepClone(task.isEnabled) : undefined, arguments: task.arguments.map((a) => deepClone(a)), executionOptions: task.executionOptions diff --git a/src/services/componentService.test.ts b/src/services/componentService.test.ts index 4815948a78..5509d85641 100644 --- a/src/services/componentService.test.ts +++ b/src/services/componentService.test.ts @@ -164,12 +164,35 @@ describe("componentService", () => { vi.mocked(localforage.componentExistsByUrl).mockResolvedValue(false); mockFetch.mockResolvedValue({ ok: false, + status: 404, headers: new Headers(), statusText: "Not Found", } as Response); const result = await fetchAndStoreComponentByUrl(url); + expect(result).toBeNull(); + expect(consoleSpy).toHaveBeenCalledWith( + `Component at URL ${url} is unavailable: Not Found`, + ); + }); + + it("should keep swallowing retryable fetch errors", async () => { + const url = "https://example.com/component.yaml"; + const consoleSpy = vi + .spyOn(console, "error") + .mockImplementation(() => {}); + + vi.mocked(localforage.componentExistsByUrl).mockResolvedValue(false); + mockFetch.mockResolvedValue({ + ok: false, + status: 503, + headers: new Headers(), + statusText: "Service Unavailable", + } as Response); + + const result = await fetchAndStoreComponentByUrl(url); + expect(result).toBeNull(); expect(consoleSpy).toHaveBeenCalledWith( `Error fetching component from URL ${url}:`, diff --git a/src/services/componentService.ts b/src/services/componentService.ts index 4604c4faa0..b4769b7f71 100644 --- a/src/services/componentService.ts +++ b/src/services/componentService.ts @@ -16,7 +16,6 @@ import { isHydratedComponentReference, isInvalidComponentReference, isLoadableComponentReference, - isNotMaterializedComponentReference, isPartialContentfulComponentReference, isSpecOnlyComponentReference, isTextOnlyComponentReference, @@ -249,6 +248,38 @@ const parseTextToSpec = async ( return null; }; +const RETRYABLE_HTTP_STATUSES = new Set([408, 425, 429]); + +const isRetryableHttpStatus = (status: number) => + status >= 500 || RETRYABLE_HTTP_STATUSES.has(status); + +const fetchComponentTextFromNetwork = async ( + url: string, +): Promise => { + const response = await fetch(url); + if (!response.ok) { + if (isRetryableHttpStatus(response.status)) { + throw new Error(`Failed to fetch component: ${response.statusText}`); + } + + console.error( + `Component at URL ${url} is unavailable: ${response.statusText}`, + ); + return undefined; + } + + // if response code is json, return the json + if (response.headers.get("content-type")?.includes("application/json")) { + const json = await response.json(); + // if coming from the Backend Component Library API + if (json.text) { + return json.text; + } + } + + return await response.text(); +}; + /** * Helper function to fetch text content from URL (with caching) */ @@ -261,29 +292,32 @@ export const fetchComponentTextFromUrl = async ( return storedComponent.data; } - // Fetch from network try { - const response = await fetch(url); - if (!response.ok) { - throw new Error(`Failed to fetch component: ${response.statusText}`); - } - - // if response code is json, return the json - if (response.headers.get("content-type")?.includes("application/json")) { - const json = await response.json(); - // if coming from the Backend Component Library API - if (json.text) { - return json.text; - } - } - - return await response.text(); + return await fetchComponentTextFromNetwork(url); } catch (error) { console.error(`Error fetching component from URL ${url}:`, error); return undefined; } }; +/** + * Same as {@link fetchComponentTextFromUrl}, but surfaces retryable failures — + * a dropped connection or a 5xx — instead of reporting them as a component with + * no content. Callers that cache their result need to tell a transient failure + * apart from a component that is genuinely gone, so a 404 still resolves to + * `undefined` rather than throwing. + */ +const fetchComponentTextFromUrlOrThrow = async ( + url: string, +): Promise => { + const storedComponent = await getComponentByUrl(url); + if (storedComponent) { + return storedComponent.data; + } + + return await fetchComponentTextFromNetwork(url); +}; + /** * Parse a component's data into a ComponentSpec */ @@ -396,17 +430,24 @@ async function hydrateFromPartialContentfulComponentReference( if (!isPartialContentfulComponentReference(component)) { return null; } - // it is ok to fail here, as we will try to fetch the text from the URL or local storage - const text = isSpecOnlyComponentReference(component) - ? componentSpecToYaml(component.spec) - : component.text; + let text: string | undefined; + let spec: ComponentSpec | undefined; - const spec = isTextOnlyComponentReference(component) - ? componentSpecFromYaml(component.text) - : component.spec; + try { + text = isSpecOnlyComponentReference(component) + ? componentSpecToYaml(component.spec) + : component.text; + + spec = isTextOnlyComponentReference(component) + ? componentSpecFromYaml(component.text) + : component.spec; + } catch { + // Unparseable content is a permanent condition — reporting it as a failure + // would have callers retry a translation that can never succeed. + return null; + } if (!text || !spec) { - // likely we should see an exception above, but for narrowing types return null; } @@ -462,6 +503,16 @@ async function saveHydratedComponentReferenceToStorage( }); } +function isUnresolvedLoadableComponentReference( + component: UnknownComponentReference, +): component is LoadableComponentReference { + return ( + isLoadableComponentReference(component) && + !isContentfulComponentReference(component) && + !isPartialContentfulComponentReference(component) + ); +} + function hydrationStrategy< T extends UnknownComponentReference = ComponentReference, >( @@ -482,7 +533,6 @@ function hydrationStrategy< /** * Hydrate a component reference by fetching the text and spec from the URL or local storage - * This is experimental function, that potentially can replace all other methods of getting ComponentRef. * * @param component - The component reference to hydrate * @returns The hydrated component reference or null if the component reference is invalid @@ -490,103 +540,93 @@ function hydrationStrategy< export const hydrateComponentReference = async ( component: ComponentReference, ): Promise => { - try { - let currentComponent: UnknownComponentReference = component; - - const strategies = [ - hydrationStrategy( - isInvalidComponentReference, - async (_: UnknownComponentReference) => null, - ), - hydrationStrategy( - isHydratedComponentReference, - async (component: HydratedComponentReference) => component, - ), - hydrationStrategy( - isContentfulComponentReference, - hydrateFromContentfulComponentReference, - ), - - hydrationStrategy( - isDiscoverableComponentReference, - async (component: DiscoverableComponentReference) => { - const storedComponent = await getComponentById( - componentId(component), - ); + let currentComponent: UnknownComponentReference = component; + + const strategies = [ + hydrationStrategy( + isInvalidComponentReference, + async (_: UnknownComponentReference) => null, + ), + hydrationStrategy( + isHydratedComponentReference, + async (component: HydratedComponentReference) => component, + ), + hydrationStrategy( + isContentfulComponentReference, + hydrateFromContentfulComponentReference, + ), + + hydrationStrategy( + isDiscoverableComponentReference, + async (component: DiscoverableComponentReference) => { + const storedComponent = await getComponentById(componentId(component)); + + if (storedComponent) { + return await hydrateFromPartialContentfulComponentReference({ + ...component, + ...normalizeStoredComponentReference(storedComponent), + }); + } - if (storedComponent) { - return await hydrateFromPartialContentfulComponentReference({ - ...component, - ...normalizeStoredComponentReference(storedComponent), - }); - } + return component; + }, + ), - return component; - }, - ), - - hydrationStrategy( - isTextOnlyComponentReference, - hydrateFromPartialContentfulComponentReference, - ), - - hydrationStrategy( - (component: UnknownComponentReference) => - isNotMaterializedComponentReference(component) && - isLoadableComponentReference(component), - async (component: LoadableComponentReference) => { - const text = await fetchComponentTextFromUrl(component.url); - - if (text) { - return ( - (await hydrateFromPartialContentfulComponentReference({ - ...component, - // errasing spec, will be restored from text to keep both in sync - spec: undefined, - text, - })) ?? - // fallback to component as is, - component - ); - } + hydrationStrategy( + isTextOnlyComponentReference, + hydrateFromPartialContentfulComponentReference, + ), - return component; - }, - ), - - hydrationStrategy( - isSpecOnlyComponentReference, - hydrateFromPartialContentfulComponentReference, - ), - ]; - - /** - * Try hydration strategies in order. - * If the component is resolved, save it to the storage and return it. - */ - for (const resolveComponentRef of strategies) { - await resolveComponentRef(currentComponent, (component) => { - currentComponent = component; - }); + hydrationStrategy( + isUnresolvedLoadableComponentReference, + async (component: LoadableComponentReference) => { + const text = await fetchComponentTextFromUrlOrThrow(component.url); - if (!currentComponent) { - return null; - } + if (text) { + return ( + (await hydrateFromPartialContentfulComponentReference({ + ...component, + // errasing spec, will be restored from text to keep both in sync + spec: undefined, + text, + })) ?? + // fallback to component as is, + component + ); + } - if (isHydratedComponentReference(currentComponent)) { - // dont wait for actualizing the cache value, as it is not critical - void saveHydratedComponentReferenceToStorage(currentComponent).catch( - // todo: handle error - console.error, - ); - return currentComponent; - } + return component; + }, + ), + + hydrationStrategy( + isSpecOnlyComponentReference, + hydrateFromPartialContentfulComponentReference, + ), + ]; + + /** + * Try hydration strategies in order. + * If the component is resolved, save it to the storage and return it. + */ + for (const resolveComponentRef of strategies) { + await resolveComponentRef(currentComponent, (component) => { + currentComponent = component; + }); + + if (!currentComponent) { + return null; } - return null; - } catch (error) { - // todo: handle error - console.error(`Error in hydrateComponentReference:`, error); - return null; + if (isHydratedComponentReference(currentComponent)) { + // dont wait for actualizing the cache value, as it is not critical + void saveHydratedComponentReferenceToStorage(currentComponent).catch( + // todo: handle error + console.error, + ); + return currentComponent; + } } + + return null; }; diff --git a/src/services/executionService.ts b/src/services/executionService.ts index 81a80c8cb5..166020b318 100644 --- a/src/services/executionService.ts +++ b/src/services/executionService.ts @@ -94,10 +94,8 @@ export const fetchContainerLog = async ( executionId: string, backendUrl: string, ): Promise => { - const response = await fetch( - `${backendUrl}/api/executions/${executionId}/container_log`, - ); - return response.json(); + const url = `${backendUrl}/api/executions/${executionId}/container_log`; + return fetchWithErrorHandling(url); }; export const useFetchContainerExecutionState = ( diff --git a/src/services/hydrateComponentReference.test.ts b/src/services/hydrateComponentReference.test.ts index e868054805..b2d960fd63 100644 --- a/src/services/hydrateComponentReference.test.ts +++ b/src/services/hydrateComponentReference.test.ts @@ -272,21 +272,20 @@ describe("hydrateComponentReference()", () => { expect(localforage.saveComponent).toHaveBeenCalled(); }); - it("should handle getComponentById throwing an error gracefully", async () => { + it("should propagate getComponentById errors so callers can retry", async () => { // Arrange const testDigest = "error123case"; const discoverableRef = { digest: testDigest }; mockGetComponentMockError(new Error("Storage error")); - // Act - const result = await hydrateComponentReference(discoverableRef); - - // Assert + // Act & Assert + await expect( + hydrateComponentReference(discoverableRef), + ).rejects.toThrow("Storage error"); expect(localforage.getComponentById).toHaveBeenCalledWith( `component-${testDigest}`, ); - expect(result).toBeNull(); }); }); }); @@ -377,7 +376,7 @@ describe("hydrateComponentReference()", () => { expect(localforage.saveComponent).toHaveBeenCalled(); }); - it("should handle fetch network errors gracefully", async () => { + it("should propagate fetch network errors so callers can retry", async () => { // Arrange const testUrl = "https://example.com/error-component.yaml"; @@ -386,22 +385,25 @@ describe("hydrateComponentReference()", () => { const loadableRef = { url: testUrl }; - // Act - const result = await hydrateComponentReference(loadableRef); - - // Assert + // Act & Assert + await expect(hydrateComponentReference(loadableRef)).rejects.toThrow( + "Network error", + ); expect(localforage.getComponentByUrl).toHaveBeenCalledWith(testUrl); expect(global.fetch).toHaveBeenCalledWith(testUrl); - expect(result).toBeNull(); expect(localforage.saveComponent).not.toHaveBeenCalled(); }); - it("should handle non-ok fetch responses", async () => { + it("should treat a missing component as unresolvable rather than retryable", async () => { // Arrange const testUrl = "https://example.com/404-component.yaml"; mockGetComponentByUrl(null); // Not cached - mockFetchResponse("", { ok: false, statusText: "Not Found" }); + mockFetchResponse("", { + ok: false, + status: 404, + statusText: "Not Found", + }); const loadableRef = { url: testUrl }; @@ -415,6 +417,27 @@ describe("hydrateComponentReference()", () => { expect(localforage.saveComponent).not.toHaveBeenCalled(); }); + it("should propagate retryable server errors so callers can retry", async () => { + // Arrange + const testUrl = "https://example.com/flaky-component.yaml"; + + mockGetComponentByUrl(null); // Not cached + mockFetchResponse("", { + ok: false, + status: 503, + statusText: "Service Unavailable", + }); + + const loadableRef = { url: testUrl }; + + // Act & Assert + await expect(hydrateComponentReference(loadableRef)).rejects.toThrow( + "Failed to fetch component: Service Unavailable", + ); + expect(global.fetch).toHaveBeenCalledWith(testUrl); + expect(localforage.saveComponent).not.toHaveBeenCalled(); + }); + it("should handle relative URL", async () => { // Arrange const testUrl = "/remote-component.yaml"; @@ -482,6 +505,33 @@ describe("hydrateComponentReference()", () => { }); }); + describe("when the reference carries an unusable spec alongside the URL", () => { + it("should still fall back to the URL", async () => { + // Arrange + const testUrl = "https://example.com/malformed-spec.yaml"; + const { text: componentText, spec: componentSpec } = + prepareComponentContent("RecoveredComponent", "recovered:v1"); + + mockGetComponentByUrl(null); + mockFetchResponse(componentText); + + const loadableRef = { + url: testUrl, + spec: { name: "Truncated" } as unknown as ComponentSpec, + }; + + // Act + const result = await hydrateComponentReference(loadableRef); + + // Assert + expect(global.fetch).toHaveBeenCalledWith(testUrl); + expect(result).not.toBeNull(); + expect(result?.name).toBe("RecoveredComponent"); + expect(result?.spec).toEqual(componentSpec); + expect(result?.text).toBe(componentText); + }); + }); + describe("when URL and digest are both present", () => { it("should prioritize digest over URL if component exists by digest", async () => { // Arrange @@ -824,7 +874,7 @@ describe("hydrateComponentReference()", () => { }); describe("error handling", () => { - it("should handle crypto.subtle.digest errors gracefully", async () => { + it("should propagate crypto.subtle.digest errors so callers can retry", async () => { // Arrange const { text: componentText } = prepareComponentContent( "ErrorComponent", @@ -837,11 +887,10 @@ describe("hydrateComponentReference()", () => { const partialRef = { text: componentText, spec: undefined }; - // Act - const result = await hydrateComponentReference(partialRef); - - // Assert - expect(result).toBeNull(); + // Act & Assert + await expect(hydrateComponentReference(partialRef)).rejects.toThrow( + "Crypto error", + ); expect(localforage.saveComponent).not.toHaveBeenCalled(); }); @@ -1034,7 +1083,7 @@ describe("hydrateComponentReference()", () => { }); }); - it("should handle crypto.subtle.digest errors gracefully", async () => { + it("should propagate crypto.subtle.digest errors so callers can retry", async () => { // Arrange const { text: componentText, spec: componentSpec } = prepareComponentContent("ErrorComponent", "error:v1"); @@ -1045,11 +1094,10 @@ describe("hydrateComponentReference()", () => { const contentfulRef = { text: componentText, spec: componentSpec }; - // Act - const result = await hydrateComponentReference(contentfulRef); - - // Assert - expect(result).toBeNull(); + // Act & Assert + await expect(hydrateComponentReference(contentfulRef)).rejects.toThrow( + "Crypto error", + ); expect(localforage.saveComponent).not.toHaveBeenCalled(); }); @@ -1323,10 +1371,11 @@ function mockGetComponentByUrl( function mockFetchResponse( text: string, - options: { ok?: boolean; statusText?: string } = {}, + options: { ok?: boolean; status?: number; statusText?: string } = {}, ) { const response = { ok: options.ok ?? true, + status: options.status ?? (options.ok === false ? 400 : 200), headers: new Headers(), statusText: options.statusText ?? "OK", text: vi.fn().mockResolvedValue(text), diff --git a/src/utils/componentSpec.ts b/src/utils/componentSpec.ts index e1fab8b051..47ec053f0a 100644 --- a/src/utils/componentSpec.ts +++ b/src/utils/componentSpec.ts @@ -449,7 +449,7 @@ export interface GraphImplementation { } export const isValidComponentSpec = (obj: any): obj is ComponentSpec => - typeof obj === "object" && "implementation" in obj; + obj !== null && typeof obj === "object" && "implementation" in obj; export const isContainerImplementation = ( implementation: ImplementationType,