From deab40e79c36f351f50a7348a01291e35c2d9b52 Mon Sep 17 00:00:00 2001 From: Tanishq Gandhi <56472134+tanishqgandhi1908@users.noreply.github.com> Date: Thu, 3 Sep 2026 22:28:36 +0000 Subject: [PATCH] fix(frontend): render workflow covers on the hub landing page (#8383) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What changes were proposed in this PR? Workflow covers never rendered on the Hub landing page: every card under **Top Loved Workflows** and **Top Cloned Workflows** showed the grey placeholder, even for a workflow whose owner had set a cover, while the same workflow showed it correctly in Your Work → Workflows. Covers reach the frontend two different ways. A **workflow** cover is a downscaled **data URL** that arrives inline on the list payload and lands on `DashboardEntry.coverImageUrl`. A **dataset** or **model** cover is a committed file, so what arrives is a *path* and the card has to fetch a presigned URL from `/{id}/cover-url`. `browse-section` only ever handled the second kind — it asks the descriptor for a `coverUrl` and bails when there is none, which is always the case for a workflow, since `WorkflowResourceDescriptor` deliberately declares none. So nothing was ever cached for a workflow and `getCoverImage` fell through to the default. `getCoverImage` now reads a workflow's cover straight off the entry, mirroring the branch `card-item.component.ts:197-202` already had. Also included, since it is one line in the same area and needs no separate issue: `frontend/proxy.config.json` declared `"/api/model/**"` **twice** (both pointing at `:9092`, so the last silently won). The duplicate is removed, leaving the entry beside `/api/dataset`, so the file reads `dataset`, `model`, `access/dataset`, `access/model`. **Before** — both workflows are public; the left one has a cover, the right one does not: issue5-1-hub-landing-before **After** — the left card renders its cover, the right one still shows the placeholder: issue5-1-hub-landing-after ### Any related issues, documentation, discussions? Closes #8382. ### How was this PR tested? `browse-section.component.spec.ts`, 25 passed: - `renders a workflow's cover from the entry, since no cover is ever fetched for one` — a workflow with a cover resolves to it, one without still gets the default. - `keeps a file-backed kind on the placeholder rather than rendering its stored cover path` — a dataset whose presigned fetch answers with an empty URL stays on the placeholder instead of rendering `v1/images/preview.png`. - `skips an entity whose descriptor resolves no cover, rather than calling undefined` was already there and asserted `getCoverImage(workflow) === defaultBackground` — it pinned the bug, so it now asserts the cover comes off the entry, with the unregistered-kind row still falling back. `landing-page.component.spec.ts` also run, 17 passed. ``` cd frontend npx ng test --include src/app/hub/component/browse-section/browse-section.component.spec.ts npx ng test --include src/app/hub/component/landing-page/landing-page.component.spec.ts ``` Checked by hand against a local stack: a public workflow with a cover set from the dashboard now shows it in both hub sections, and a public workflow without one is unchanged. ### Was this PR authored or co-authored using generative AI tooling? (backported from commit 1facefb183c96e7b426ff6be35e5b6aaa7b33345) Generated-by: Claude Code (Opus 5) --- frontend/proxy.config.json | 17 ++ .../browse-section.component.spec.ts | 199 ++++++++++++++++++ .../browse-section.component.ts | 16 ++ 3 files changed, 232 insertions(+) diff --git a/frontend/proxy.config.json b/frontend/proxy.config.json index 6802ae762c4..2c0b17512bc 100755 --- a/frontend/proxy.config.json +++ b/frontend/proxy.config.json @@ -10,7 +10,16 @@ "secure": false, "changeOrigin": true }, +<<<<<<< HEAD "/api/models": { +======= + "/api/notebook-migration": { + "target": "http://localhost:9098", + "secure": false, + "changeOrigin": true + }, + "/api/models": { +>>>>>>> 1facefb18 (fix(frontend): render workflow covers on the hub landing page (#8383)) "target": "http://localhost:9096", "secure": false, "changeOrigin": true @@ -35,6 +44,14 @@ "secure": false, "changeOrigin": true }, +<<<<<<< HEAD +======= + "/api/access/model/**": { + "target": "http://localhost:9092", + "secure": false, + "changeOrigin": true + }, +>>>>>>> 1facefb18 (fix(frontend): render workflow covers on the hub landing page (#8383)) "/api/access/computing-unit/**": { "target": "http://localhost:8888", "secure": false, diff --git a/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts b/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts index 504101e32e0..a27c63c2072 100644 --- a/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts +++ b/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts @@ -82,5 +82,204 @@ describe("BrowseSectionComponent", () => { component.ngOnInit(); expect(component.entityRoutes[201]).toEqual([HUB_DATASET_RESULT_DETAIL, "201"]); }); +<<<<<<< HEAD +======= + + it("falls back to the default background when no cover was cached", () => { + // No coverImageUrl -> loadCoverImages never asks the descriptor -> getCoverImage defaults. + const entity = { id: 6, type: "dataset", accessibleUserIds: [] } as unknown as DashboardEntry; + component.entities = [entity]; + component.ngOnInit(); + + expect(component.getCoverImage(entity)).toBe(component.defaultBackground); + }); + + // `this.resourceRegistry.find(entity.type)?.coverUrl` carries two guards, and a mixed section + // can trip either. A workflow's cover is a data URL carried on the entry itself, so + // WorkflowResourceDescriptor deliberately declares no `coverUrl`; and a kind the registry does + // not carry at all has no descriptor to ask, which is why this is `find`, not `get` — one such + // row must not take the whole section's covers down, exactly as `routeFor` five lines up + // already promises for links. + it("skips an entity whose descriptor resolves no cover, rather than calling undefined", () => { + const workflow = { + id: 10, + type: "workflow", + coverImageUrl: "carried-on-the-entry", + accessibleUserIds: [], + } as unknown as DashboardEntry; + const unregistered = { + id: 12, + type: "computing-unit", + coverImageUrl: "carried-on-the-entry", + accessibleUserIds: [], + } as unknown as DashboardEntry; + component.entities = [workflow, unregistered]; + + expect(() => component.ngOnInit()).not.toThrow(); + expect(coverCache(component).has("workflow:10")).toBe(false); + expect(coverCache(component).has("computing-unit:12")).toBe(false); + // Nothing is cached for a workflow, but its cover is readable straight off the entry. + expect(component.getCoverImage(workflow)).toBe("carried-on-the-entry"); + expect(component.getCoverImage(unregistered)).toBe(component.defaultBackground); + }); + + it("renders a workflow's cover from the entry, since no cover is ever fetched for one", () => { + const withCover = { + id: 20, + type: "workflow", + coverImageUrl: "data:image/png;base64,AAAA", + accessibleUserIds: [], + } as unknown as DashboardEntry; + const withoutCover = { id: 21, type: "workflow", accessibleUserIds: [] } as unknown as DashboardEntry; + component.entities = [withCover, withoutCover]; + component.ngOnInit(); + + expect(component.getCoverImage(withCover)).toBe("data:image/png;base64,AAAA"); + expect(component.getCoverImage(withoutCover)).toBe(component.defaultBackground); + }); + + it("keeps a file-backed kind on the placeholder rather than rendering its stored cover path", () => { + // A dataset's coverImageUrl is a path relative to the dataset root, not something an + // can load, so it must never stand in for the presigned URL the descriptor resolves. + vi.spyOn(TestBed.inject(DatasetService) as any, "getDatasetCoverUrl").mockReturnValue(of({ url: "" })); + const entity = { + id: 22, + type: "dataset", + coverImageUrl: "v1/images/preview.png", + accessibleUserIds: [], + } as unknown as DashboardEntry; + component.entities = [entity]; + component.ngOnInit(); + + expect(component.getCoverImage(entity)).toBe(component.defaultBackground); + }); + + it("caches nothing when the descriptor resolves an empty cover url", () => { + // A presigned-URL endpoint with nothing to sign answers with an empty string; caching that + // would put an on the card, which the browser resolves to the page itself. + vi.spyOn(TestBed.inject(DatasetService) as any, "getDatasetCoverUrl").mockReturnValue(of({ url: "" })); + const entity = { + id: 11, + type: "dataset", + coverImageUrl: "has-cover", + accessibleUserIds: [], + } as unknown as DashboardEntry; + component.entities = [entity]; + component.ngOnInit(); + + // White-box on purpose: getCoverImage's `|| defaultBackground` makes "cached an empty string" + // and "cached nothing" indistinguishable through the public API, so only the map itself can + // say whether the guard ran. + expect(coverCache(component).has("dataset:11")).toBe(false); + expect(component.getCoverImage(entity)).toBe(component.defaultBackground); + }); + + /** The component's cover cache, which no public member exposes. */ + function coverCache(c: BrowseSectionComponent): Map { + return (c as unknown as { coverImageUrls: Map }).coverImageUrls; + } + }); +}); +/** + * The cards themselves are template-only: the specs above assert the route map and the cover-URL + * cache, but nothing had ever rendered a card, so the per-entity bindings and their fallbacks were + * unpinned. RouterTestingModule supplies the Router that the cards' routerLink needs. + */ +describe("BrowseSectionComponent rendering", () => { + let fixture: ComponentFixture; + + beforeEach(() => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + imports: [BrowseSectionComponent, RouterTestingModule.withRoutes([])], + providers: [ + // The cards embed texera-user-avatar, which injects UserService; the real one drags in + // AuthService and its whole dependency chain, so the shared stub stands in for it. + { provide: UserService, useClass: StubUserService }, + { provide: WorkflowPersistService, useValue: {} }, + // The cover now comes from the descriptor, so the double has to answer for it. + { provide: DatasetService, useValue: { getDatasetCoverUrl: () => of({ url: PRESIGNED_COVER }) } }, + { provide: ModelService, useValue: { getModelCoverUrl: () => of({ url: PRESIGNED_COVER }) } }, + ...commonTestProviders, + ], + }); + fixture = TestBed.createComponent(BrowseSectionComponent); + }); + + /** Renders the section with the given entities. */ + function render(entities: DashboardEntry[], title = "Workflows"): HTMLElement { + // Set the inputs and let the first change-detection cycle drive ngOnInit, as Angular does at + // runtime. Calling ngOnInit() by hand as well would run it twice and rebuild the cover-image + // cache on top of itself, hiding any non-idempotent init. + fixture.componentRef.setInput("entities", entities); + fixture.componentRef.setInput("sectionTitle", title); + fixture.detectChanges(); + return fixture.nativeElement as HTMLElement; + } + + const entity = (over: Partial> = {}) => + ({ id: 1, type: "dataset", accessibleUserIds: [], name: "flow", ...over }) as unknown as DashboardEntry; + + it("renders nothing at all for an empty section", () => { + const el = render([]); + + expect(el.querySelector(".results-container")).toBeNull(); + }); + + it("renders the section heading and one card per entity", () => { + const el = render([entity({ id: 1 }), entity({ id: 2 })], "Public Datasets"); + + expect(el.querySelector(".results-title")?.textContent?.trim()).toBe("Public Datasets"); + expect(el.querySelectorAll("nz-card")).toHaveLength(2); + }); + + it("shows each entity's name and description", () => { + const el = render([entity({ name: "sales", description: "quarterly numbers" })]); + + expect(el.querySelector(".card-title")?.textContent?.trim()).toBe("sales"); + expect(el.querySelector(".card-description")?.textContent?.trim()).toBe("quarterly numbers"); + }); + + it("substitutes a placeholder for a missing description", () => { + // Datasets published without a description would otherwise render an empty paragraph and + // collapse the card's layout. + const el = render([entity({ description: undefined })]); + + expect(el.querySelector(".card-description")?.textContent?.trim()).toBe("No description available"); + }); + + it("uses the cached cover image when the entity has one", () => { + const el = render([entity({ id: 5, coverImageUrl: "has-cover" })]); + + const img = el.querySelector(".card-cover-image")!; + expect(img.getAttribute("src")).toBe(PRESIGNED_COVER); + }); + + it("falls back to the default background when the cover image fails to load", () => { + // A presigned cover URL can still 404; the inline error handler is the only thing that stops the + // card from showing a broken image. + const el = render([entity({ id: 5, coverImageUrl: "has-cover" })]); + const img = el.querySelector(".card-cover-image")!; + + img.dispatchEvent(new Event("error")); + + expect(img.src).toContain("card_background.jpg"); + }); + + it("labels the avatar with the entity id", () => { + const el = render([entity({ id: 42 })]); + + expect(el.querySelector("nz-avatar")?.textContent?.trim()).toBe("42"); + }); + + it("passes the owner through to the avatar, defaulting to an empty name", () => { + const withOwner = fixture.debugElement.queryAll(By.css("texera-user-avatar")); + expect(withOwner).toHaveLength(0); + + render([entity({ ownerName: "ada" }), entity({ id: 2, ownerName: undefined })]); + + const avatars = fixture.debugElement.queryAll(By.css("texera-user-avatar")); + expect(avatars.map(a => a.componentInstance.userName)).toEqual(["ada", ""]); +>>>>>>> 1facefb18 (fix(frontend): render workflow covers on the hub landing page (#8383)) }); }); diff --git a/frontend/src/app/hub/component/browse-section/browse-section.component.ts b/frontend/src/app/hub/component/browse-section/browse-section.component.ts index 659e018e1e1..bc7d806884f 100644 --- a/frontend/src/app/hub/component/browse-section/browse-section.component.ts +++ b/frontend/src/app/hub/component/browse-section/browse-section.component.ts @@ -19,6 +19,7 @@ import { ChangeDetectorRef, Component, Input, OnChanges, OnInit, SimpleChanges } from "@angular/core"; import { DashboardEntry } from "../../../dashboard/type/dashboard-entry"; +<<<<<<< HEAD import { WorkflowPersistService } from "../../../common/service/workflow-persist/workflow-persist.service"; import { DatasetService } from "../../../dashboard/service/user/dataset/dataset.service"; import { UntilDestroy } from "@ngneat/until-destroy"; @@ -29,6 +30,11 @@ import { USER_WORKSPACE, } from "../../../app-routing.constant"; import { AppSettings } from "../../../common/app-setting"; +======= +import { EntityType } from "../../service/hub.service"; +import { ResourceRegistryService } from "../../../dashboard/service/user/resource-registry/resource-registry.service"; +import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; +>>>>>>> 1facefb18 (fix(frontend): render workflow covers on the hub landing page (#8383)) import { NgIf, NgFor, NgStyle, DatePipe } from "@angular/common"; import { NzCardComponent } from "ng-zorro-antd/card"; import { RouterLink } from "@angular/router"; @@ -130,6 +136,16 @@ export class BrowseSectionComponent implements OnInit, OnChanges { } getCoverImage(entity: DashboardEntry): string { +<<<<<<< HEAD return this.coverImageUrls.get(entity.id!) || this.defaultBackground; +======= + // A workflow's cover is a downscaled data URL carried on the entry, so nothing is ever fetched + // for it. The file-backed kinds carry a stored path instead, which only the cache above can + // turn into something an can load. + if (entity.type === EntityType.Workflow) { + return entity.coverImageUrl ?? this.defaultBackground; + } + return this.coverImageUrls.get(this.cacheKey(entity)) || this.defaultBackground; +>>>>>>> 1facefb18 (fix(frontend): render workflow covers on the hub landing page (#8383)) } }