aglinxinyuan commented on code in PR #5704:
URL: https://github.com/apache/texera/pull/5704#discussion_r3457214123
##########
frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.ts:
##########
@@ -131,12 +136,64 @@ export class CardItemComponent implements OnChanges {
private hubService: HubService,
private downloadService: DownloadService,
private cdr: ChangeDetectorRef,
- private notificationService: NotificationService
+ private notificationService: NotificationService,
+ private workflowCoverService: WorkflowCoverService
) {}
- /** The top image src for the card preview. */
+ /** The top image src: the user's custom cover if present, otherwise the
default. */
get previewImage(): string {
- return CardItemComponent.DEFAULT_PREVIEW_IMAGE;
+ return this.customImage ?? CardItemComponent.DEFAULT_PREVIEW_IMAGE;
+ }
+
+ get hasCustomImage(): boolean {
+ return this.customImage !== undefined;
+ }
+
+ /** Whether the cover-image controls are shown: an editable workflow in
private search. */
+ get canEditCover(): boolean {
+ return this.isPrivateSearch && this.entry.type === "workflow" &&
this.entry.workflow.isOwner;
+ }
+
+ openImagePicker(): void {
+ this.backgroundInput?.nativeElement.click();
+ }
+
+ async onImageSelected(event: Event): Promise<void> {
+ const input = event.target as HTMLInputElement;
+ const file = input.files?.[0];
+ input.value = ""; // allow re-selecting the same file
+ if (!file) {
+ return;
+ }
+ if (!file.type.startsWith("image/")) {
+ this.notificationService.error("Please choose an image file.");
+ return;
+ }
+ if (typeof this.entry.id !== "number") {
+ return;
+ }
+ try {
+ this.customImage = await
this.workflowCoverService.setCoverFromFile(this.entry.id, file);
+ this.cdr.markForCheck();
+ } catch (e) {
+ this.notificationService.error("Failed to set the cover image.");
+ }
+ }
Review Comment:
Non-blocking — this path is promise/`await` with no `untilDestroyed`,
whereas `resetImage` uses the Observable + `untilDestroyed` pattern. If the
card is destroyed mid-upload we'd still touch `customImage`/`markForCheck()` on
a dead component. Worth aligning the two for consistency.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -711,6 +730,75 @@ class WorkflowResource extends LazyLogging {
workflowDao.update(workflow)
}
+ /** Returns the workflow's cover image; 404 if none set. */
+ @GET
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{wid}/cover")
+ def getCoverImage(@PathParam("wid") wid: Integer, @Auth user: SessionUser):
CoverImageRequest = {
Review Comment:
Tiny nit — `CoverImageRequest` is doubling as the GET response type here. A
`CoverImageResponse` (or a neutral `CoverImage`) would read a bit cleaner.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -711,6 +730,75 @@ class WorkflowResource extends LazyLogging {
workflowDao.update(workflow)
}
+ /** Returns the workflow's cover image; 404 if none set. */
+ @GET
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{wid}/cover")
+ def getCoverImage(@PathParam("wid") wid: Integer, @Auth user: SessionUser):
CoverImageRequest = {
+ if (!WorkflowAccessResource.hasReadAccess(wid, user.getUid)) {
+ throw new ForbiddenException(s"You do not have access to workflow $wid")
+ }
+ val image = context
+ .select(WORKFLOW_COVER_IMAGE.IMAGE)
+ .from(WORKFLOW_COVER_IMAGE)
+ .where(WORKFLOW_COVER_IMAGE.WID.eq(wid))
+ .fetchOne(WORKFLOW_COVER_IMAGE.IMAGE)
+ if (image == null) {
+ throw new NotFoundException(s"Workflow $wid has no cover image")
+ }
+ CoverImageRequest(image)
+ }
+
+ /** Sets or replaces the workflow's cover image. */
+ @PUT
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{wid}/cover")
+ def setCoverImage(
+ @PathParam("wid") wid: Integer,
+ request: CoverImageRequest,
+ @Auth user: SessionUser
+ ): Unit = {
+ if (!WorkflowAccessResource.hasWriteAccess(wid, user.getUid)) {
Review Comment:
Worth confirming intent: set/delete authorize on `hasWriteAccess`, so any
WRITE collaborator can change the cover — and since it's stored per-workflow,
that changes it for everyone — while the UI only exposes the controls to the
owner (`canEditCover`). The backend being broader than the UI is fine; I just
want to make sure a shared, collaborator-editable cover is what #5703 intends
(vs. per-user).
##########
frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.ts:
##########
@@ -150,6 +207,13 @@ export class CardItemComponent implements OnChanges {
this.entryLink = [HUB_WORKFLOW_RESULT_DETAIL, String(this.entry.id)];
}
this.size = this.entry.size;
+ this.workflowCoverService
+ .getCover(this.entry.id)
+ .pipe(untilDestroyed(this))
+ .subscribe(image => {
+ this.customImage = image;
+ this.cdr.markForCheck();
+ });
Review Comment:
`getCover` fires here once per workflow card inside `initializeEntry`, so a
dashboard/hub page of N cards issues N separate `GET /{wid}/cover` calls — each
a DB hit + access check — re-fired on every `ngOnChanges`, with no caching.
That'll scale poorly on a busy dashboard. Could we fold the cover (or just a
`hasCover` flag to gate a lazy fetch) into the existing workflow-list/search
payload instead?
Minor, same block: `customImage` isn't reset before the async result lands,
so when Angular reuses a card instance for a different entry the previous cover
flashes until the new fetch resolves — and overlapping `ngOnChanges`
subscriptions can resolve out of order. A `switchMap` would address both.
##########
frontend/src/app/dashboard/service/user/workflow-cover/workflow-cover.service.ts:
##########
@@ -0,0 +1,87 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { HttpClient } from "@angular/common/http";
+import { Injectable } from "@angular/core";
+import { Observable, firstValueFrom, of } from "rxjs";
+import { catchError, map } from "rxjs/operators";
+import { AppSettings } from "../../../../common/app-setting";
+
+export const WORKFLOW_COVER_URL = "workflow";
+
+/** Longest edge (px) a custom cover image is downscaled to before being
stored. */
+const MAX_IMAGE_EDGE = 640;
+/** JPEG quality used when re-encoding a custom cover image for storage. */
+const IMAGE_QUALITY = 0.8;
+
+/** Stores an optional custom cover image per workflow on the backend,
downscaled and re-encoded as a JPEG data URL. */
+@Injectable({
+ providedIn: "root",
+})
+export class WorkflowCoverService {
+ constructor(private http: HttpClient) {}
+
+ /** The workflow's custom cover image data URL, or undefined if it has none.
*/
+ getCover(wid: number): Observable<string | undefined> {
+ return this.http.get<{ image: string
}>(`${AppSettings.getApiEndpoint()}/${WORKFLOW_COVER_URL}/${wid}/cover`).pipe(
+ map(response => response.image),
+ catchError(() => of(undefined))
Review Comment:
Minor — this swallows every error to `undefined`, not just 404, so a
transient 500 silently shows the default cover with no signal to the user. Fine
if intentional; just flagging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
