Track image generation method and display in UI#347
Conversation
Persist the AI-generated scene description on each example image and surface it in the image grid: a 'direct'/'described' label is shown next to the model name, with the description revealed on hover via tooltip.
Code ReviewOverviewThis PR tracks how example images were generated (directly from input or via an intermediate description/scene) and surfaces that information in the UI with "direct"/"described" labels and tooltips. The approach is clean and the feature is well-scoped. Issues1. Template and CSS duplication between carousel and grid componentsThe model-name block with the generation-method span is identical in both 2.
|
- Extract the model-name/method label into a reusable ImageModelBadgeComponent shared by the image grid and carousel, removing duplicated template and CSS. - Rename the description variable in ImageService for clarity. - Add tooltip and persisted-description assertions to the OpenAI described test for parity with the Gemini test.
Assert the auto-dismissing 'Card updated successfully' toast immediately after generation instead of after the slow image download/decode, where it may have already vanished. Use exact text matching for the direct/described method labels.
…ace-1r89qt # Conflicts: # client/src/app/shared/types/image-generation.types.ts # server/src/main/java/io/github/mucsi96/learnlanguage/controller/ImageController.java
Code ReviewOverviewThis PR tracks how images were generated (directly vs. via scene description) by adding a Positive Aspects
Issues / SuggestionsBackend
The prompt selection now indirectly uses // Current
final String sceneDescription = describe ? describeScene(input, context) : null;
final String prompt = sceneDescription != null ? sceneDescription : input;
// Clearer — condition matches the `describe` flag directly
final String sceneDescription = describe ? describeScene(input, context) : null;
final String prompt = describe ? sceneDescription : input;More importantly: if Frontend
[matTooltip]="description() ?? ''"
[matTooltipDisabled]="!description()"The [matTooltip]="description()"
[matTooltipDisabled]="!description()"Tests Missing negative tooltip assertion for "direct" images The test verifies hovering on "described" shows a tooltip, but doesn't verify that hovering on "direct" does not show a tooltip (or shows nothing). Given that await page.getByText('direct', { exact: true }).first().hover();
await expect(page.getByRole('tooltip')).not.toBeVisible();SummaryOverall this is a clean, well-structured change. The data flow is correct end-to-end and the component extraction is a net improvement. The two main suggestions are: prefer |
…ltip test Branch the image prompt on the describe flag directly rather than on whether a scene description was produced, avoiding a silent fallback to a direct prompt. Add an assertion that direct images show no description tooltip.
Code ReviewOverviewThis PR adds tracking of how images were generated ("direct" vs "described" via AI scene description) and surfaces this in the UI with tooltips. The implementation is clean and well-scoped. Positives
Issues / SuggestionsMinor: Redundant ternary in
|
Material renders tooltips with aria-hidden, so getByRole('tooltip') never
matches them. Assert the tooltip's visible text via getByText on the
description's distinctive phrase instead, and make the direct-image negative
check assert no description text appears.
Code ReviewOverviewThis PR cleanly tracks how images were generated (direct prompt vs. AI-described scene) and surfaces that in the UI via a new IssuesBug:
|
The help cursor was applied to all method badges, but direct badges have no tooltip; gate it on the presence of a description.
Code ReviewOverviewThis PR adds tracking of image generation method ("direct" vs "described") and surfaces it in the UI via a new reusable Positives
IssuesMinor: Redundant variable in final String sceneDescription = describe ? describeScene(input, context) : null;
final String prompt = describe ? sceneDescription : input; // re-checks describe flagThe second line re-checks final String sceneDescription = describe ? describeScene(input, context) : null;
final String prompt = sceneDescription != null ? sceneDescription : input;This makes the intent clearer — use the description as the prompt when it exists. Test: Checking for tooltip absence is too narrow await page.getByText('direct', { exact: true }).first().hover();
await expect(page.getByText('Detailed scene:')).toHaveCount(0);This only asserts the absence of the string await expect(page.getByRole('tooltip')).toHaveCount(0);Test: AI-generated content assertion is fragile await expect(page.getByText('A train platform with a large clock')).toBeVisible();This asserts specific wording from an AI-generated description, which could be non-deterministic. If the model returns semantically equivalent but differently-worded output, the test will flake. Consider asserting that some description tooltip is visible (e.g., via Missing @Component({
selector: 'app-image-model-badge',
imports: [MatTooltipModule],
...
})
export class ImageModelBadgeComponent {Angular 19+ defaults to standalone, so this works, but all other components in the project that are visible in the diff explicitly declare SummaryThe implementation is solid — clean abstraction, correct data flow end-to-end, and good test coverage for the happy path. The two test concerns (fragile AI content assertion and narrow tooltip absence check) are the main things worth addressing before merge. |
Code ReviewOverviewThis PR tracks image generation method (direct vs. described) through the full stack — from backend job entity to a new frontend Positives
Issues & Suggestions
final String sceneDescription = describe ? describeScene(input, context) : null;
final String prompt = describe ? sceneDescription : input;The final String sceneDescription = describe ? describeScene(input, context) : null;
final String prompt = sceneDescription != null ? sceneDescription : input;This eliminates the implicit coupling between Fragile tooltip assertions in tests await expect(
page.getByText('Detailed scene: Wann fährt der Zug ab? A train platform with a large clock, no visible text.').first()
).toBeVisible();This asserts on AI-generated content verbatim. If this is mocked in the test environment, it's fine — but if it hits a real model it will be non-deterministic. Verify the test mock returns this exact string; if not, assert on a stable prefix or a partial match with
[matTooltip]="description() ?? ''"
[matTooltipDisabled]="!description()"This is correct and safe, but No Issues Found In
SummarySolid PR. The only actionable items are the minor redundancy in |
2 participants
Summary
This change adds tracking of how images were generated (directly or via description) and displays this information in the UI with tooltips showing the description when applicable.
Key Changes
ImageService.generateImage()to return a newGeneratedImageobject containing both the image data and an optional description, instead of just the byte arrayGeneratedImagemodel class to encapsulate image data and descriptionImageControllerto extract and store the description inExampleImageDataExampleImageDatamodel to include optionaldescriptionfieldCarouselImage,GridImageValue,ExampleImage, andImageResponseto include optionaldescriptionfieldMatTooltipModuleimports to carousel and grid componentsImplementation Details
describeScene()when thedescribeflag is true, otherwise it remains nullhttps://claude.ai/code/session_01PYCevjj6DcZL7kAUS5xtRG