-
Notifications
You must be signed in to change notification settings - Fork 622
docs(assets): unify display_name helper docstrings + fixture tests (FE-747) #12399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
2184d25
e35bb25
2fd6725
f0299e5
283e741
edeeb2d
c52dcfc
1935b52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -176,9 +176,11 @@ export function getAssetFilename(asset: AssetItem): string { | |
| * Fallback chain: user_metadata.filename → metadata.filename → | ||
| * asset.display_name → asset.name. | ||
| * | ||
| * `display_name` is populated by queue output mappers in Cloud where | ||
| * `asset.name` is a content hash. Use this helper for labels/titles only; | ||
| * for serialized identifiers use {@link getAssetFilename}. | ||
| * `display_name` is the unified user-facing label emitted by both Core | ||
| * and Cloud per the BE-808 Asset Identity RFC — required where | ||
| * `asset.name` is a content hash (hash-keyed assets). Use this helper | ||
| * for labels/titles only; for serialized identifiers use | ||
| * {@link getAssetFilename}. | ||
| */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the trimmed docstring loses the useful "queue output mappers in Cloud populate
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intentionally left as-is. The empirical Cloud probe (PR description §6) showed |
||
| export function getAssetDisplayFilename(asset: AssetItem): string { | ||
| return ( | ||
|
|
@@ -191,7 +193,7 @@ export function getAssetDisplayFilename(asset: AssetItem): string { | |
| * Prefers a user-curated name (user_metadata.name / metadata.name) when it | ||
| * actually differs from asset.name, so a user-renamed model keeps its | ||
| * display name. Falls through to {@link getAssetDisplayFilename} when the | ||
| * curated name is absent or equal to asset.name (Cloud hash case). | ||
| * curated name is absent or equal to asset.name (hash-keyed asset case). | ||
| */ | ||
| export function getAssetCardTitle(asset: AssetItem): string { | ||
| const curatedName = getStringProperty(asset, 'name') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,212 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| import type { AssetItem } from '@/platform/assets/schemas/assetSchema' | ||
| import type { MissingMediaCandidate } from '@/platform/missingMedia/types' | ||
| import { | ||
| getMediaDisplayName, | ||
| useMissingMediaInteractions | ||
| } from '@/platform/missingMedia/composables/useMissingMediaInteractions' | ||
|
|
||
| const mockInputAssetsByFilename = new Map<string, AssetItem>() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: module-level mutable
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — switched to a single |
||
|
|
||
| vi.mock('@/stores/assetsStore', () => ({ | ||
| useAssetsStore: () => ({ | ||
| inputAssetsByFilename: mockInputAssetsByFilename, | ||
| updateInputs: vi.fn() | ||
| }) | ||
| })) | ||
|
|
||
| vi.mock('@/platform/missingMedia/missingMediaStore', () => ({ | ||
| useMissingMediaStore: () => ({ | ||
| expandState: {}, | ||
| pendingSelection: {}, | ||
| uploadState: {}, | ||
| missingMediaCandidates: null, | ||
| removeMissingMediaByName: vi.fn() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropped |
||
| }) | ||
| })) | ||
|
|
||
| const mockGetNodeByExecutionId = vi.fn() | ||
| vi.mock('@/utils/graphTraversalUtil', () => ({ | ||
| getNodeByExecutionId: (...args: unknown[]) => | ||
| mockGetNodeByExecutionId(...args) | ||
| })) | ||
|
|
||
| const mockResolveComboValues = vi.fn() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: same hoisting story for
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — |
||
| vi.mock('@/utils/litegraphUtil', () => ({ | ||
| resolveComboValues: (widget: unknown) => mockResolveComboValues(widget), | ||
| addToComboValues: vi.fn() | ||
| })) | ||
|
|
||
| vi.mock('@/scripts/app', () => ({ | ||
| app: { | ||
| rootGraph: { id: 'mock-graph' } | ||
| } | ||
| })) | ||
|
|
||
| vi.mock('@/scripts/api', () => ({ | ||
| api: { fetchApi: vi.fn() } | ||
| })) | ||
|
|
||
| vi.mock('@/platform/updates/common/toastStore', () => ({ | ||
| useToastStore: () => ({ addAlert: vi.fn() }) | ||
| })) | ||
|
|
||
| const baseAsset: AssetItem = { | ||
| id: 'asset-1', | ||
| name: '', | ||
| tags: ['input'], | ||
| size: 1024 | ||
| } | ||
|
|
||
| describe('getMediaDisplayName', () => { | ||
| beforeEach(() => { | ||
| mockInputAssetsByFilename.clear() | ||
| }) | ||
|
|
||
| it('returns the input string when no matching asset is in the store (OSS pass-through)', () => { | ||
| expect(getMediaDisplayName('sunset.png')).toBe('sunset.png') | ||
| }) | ||
|
|
||
| it('returns display_name when the matched asset carries one (Cloud unified shape)', () => { | ||
| const hash = 'blake3:abc1234567890def.png' | ||
| mockInputAssetsByFilename.set(hash, { | ||
| ...baseAsset, | ||
| name: hash, | ||
| asset_hash: hash, | ||
| display_name: 'sunset.png' | ||
| }) | ||
| expect(getMediaDisplayName(hash)).toBe('sunset.png') | ||
| }) | ||
|
|
||
| it('falls back to asset.name when display_name is absent (legacy Cloud asset)', () => { | ||
| const hash = 'blake3:def4567890abc1234.png' | ||
| mockInputAssetsByFilename.set(hash, { | ||
| ...baseAsset, | ||
| name: 'beach.png', | ||
| asset_hash: hash | ||
| }) | ||
| expect(getMediaDisplayName(hash)).toBe('beach.png') | ||
| }) | ||
|
|
||
| it('prefers metadata.filename over display_name and asset.name (shared helper chain)', () => { | ||
| const hash = 'blake3:fff1111222.png' | ||
| mockInputAssetsByFilename.set(hash, { | ||
| ...baseAsset, | ||
| name: hash, | ||
| asset_hash: hash, | ||
| display_name: 'from_display.png', | ||
| metadata: { filename: 'from_metadata.png' } | ||
| }) | ||
| expect(getMediaDisplayName(hash)).toBe('from_metadata.png') | ||
| }) | ||
|
|
||
| it('falls back to display_name when filename metadata is absent (Cloud hash-keyed asset)', () => { | ||
| const hash = 'blake3:aaa2222333.png' | ||
| mockInputAssetsByFilename.set(hash, { | ||
| ...baseAsset, | ||
| name: hash, | ||
| asset_hash: hash, | ||
| display_name: 'pretty.png' | ||
| }) | ||
| expect(getMediaDisplayName(hash)).toBe('pretty.png') | ||
| }) | ||
| }) | ||
|
|
||
| describe('getLibraryOptions (integration with getMediaDisplayName)', () => { | ||
| const makeCandidate = ( | ||
| overrides: Partial<MissingMediaCandidate> = {} | ||
| ): MissingMediaCandidate => ({ | ||
| nodeId: 1, | ||
| nodeType: 'LoadImage', | ||
| widgetName: 'image', | ||
| mediaType: 'image', | ||
| name: 'missing.png', | ||
| isMissing: true, | ||
| ...overrides | ||
| }) | ||
|
|
||
| const makeNode = (widgetType: string = 'combo') => ({ | ||
| widgets: [ | ||
| { | ||
| name: 'image', | ||
| type: widgetType, | ||
| value: '', | ||
| options: {} | ||
| } | ||
| ] | ||
| }) | ||
|
|
||
| beforeEach(() => { | ||
| mockInputAssetsByFilename.clear() | ||
| mockGetNodeByExecutionId.mockReset() | ||
| mockResolveComboValues.mockReset() | ||
| }) | ||
|
|
||
| it('returns empty array when the combo widget cannot be resolved', () => { | ||
| mockGetNodeByExecutionId.mockReturnValue(null) | ||
| const { getLibraryOptions } = useMissingMediaInteractions() | ||
|
|
||
| expect(getLibraryOptions(makeCandidate())).toEqual([]) | ||
| expect(mockResolveComboValues).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('maps Cloud hash combo values to display_name via the shared helper chain', () => { | ||
| const candidateName = 'blake3:missing.png' | ||
| const hashA = 'blake3:aaa.png' | ||
| const hashB = 'blake3:bbb.png' | ||
| mockInputAssetsByFilename.set(hashA, { | ||
| ...baseAsset, | ||
| name: hashA, | ||
| asset_hash: hashA, | ||
| display_name: 'sunset.png' | ||
| }) | ||
| mockInputAssetsByFilename.set(hashB, { | ||
| ...baseAsset, | ||
| name: hashB, | ||
| asset_hash: hashB, | ||
| metadata: { filename: 'beach.png' } | ||
| }) | ||
| mockGetNodeByExecutionId.mockReturnValue(makeNode()) | ||
| mockResolveComboValues.mockReturnValue([hashA, hashB, candidateName]) | ||
|
|
||
| const { getLibraryOptions } = useMissingMediaInteractions() | ||
| const options = getLibraryOptions(makeCandidate({ name: candidateName })) | ||
|
|
||
| expect(options).toEqual([ | ||
| { name: 'sunset.png', value: hashA }, | ||
| { name: 'beach.png', value: hashB } | ||
| ]) | ||
| }) | ||
|
|
||
| it('passes OSS filename combo values through when no matching asset exists', () => { | ||
| mockGetNodeByExecutionId.mockReturnValue(makeNode()) | ||
| mockResolveComboValues.mockReturnValue([ | ||
| 'kitten.png', | ||
| 'puppy.png', | ||
| 'missing.png' | ||
| ]) | ||
|
|
||
| const { getLibraryOptions } = useMissingMediaInteractions() | ||
| const options = getLibraryOptions(makeCandidate({ name: 'missing.png' })) | ||
|
|
||
| expect(options).toEqual([ | ||
| { name: 'kitten.png', value: 'kitten.png' }, | ||
| { name: 'puppy.png', value: 'puppy.png' } | ||
| ]) | ||
| }) | ||
|
|
||
| it('filters out the candidate name from the alternatives list', () => { | ||
| mockGetNodeByExecutionId.mockReturnValue(makeNode()) | ||
| mockResolveComboValues.mockReturnValue([ | ||
| 'other.png', | ||
| 'missing.png', | ||
| 'extra.png' | ||
| ]) | ||
|
|
||
| const { getLibraryOptions } = useMissingMediaInteractions() | ||
| const options = getLibraryOptions(makeCandidate({ name: 'missing.png' })) | ||
|
|
||
| expect(options.map((o) => o.value)).toEqual(['other.png', 'extra.png']) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,14 @@ | ||
| import { app } from '@/scripts/app' | ||
| import { api } from '@/scripts/api' | ||
| import { useToastStore } from '@/platform/updates/common/toastStore' | ||
| import { getAssetDisplayFilename } from '@/platform/assets/utils/assetMetadataUtils' | ||
| import { useAssetsStore } from '@/stores/assetsStore' | ||
| import { useMissingMediaStore } from '@/platform/missingMedia/missingMediaStore' | ||
| import type { | ||
| MissingMediaCandidate, | ||
| MediaType | ||
| } from '@/platform/missingMedia/types' | ||
| import { getNodeByExecutionId } from '@/utils/graphTraversalUtil' | ||
| import { isCloud } from '@/platform/distribution/types' | ||
| import { addToComboValues, resolveComboValues } from '@/utils/litegraphUtil' | ||
| import { resolveNodeDisplayName } from '@/utils/nodeTitleUtil' | ||
| import { st } from '@/i18n' | ||
|
|
@@ -86,12 +86,17 @@ export function getNodeDisplayLabel( | |
|
|
||
| /** | ||
| * Resolve display name for a media file. | ||
| * Cloud widgets store asset hashes as values; this resolves them to | ||
| * human-readable names via assetsStore.getInputName(). | ||
| * Widget values may be content hashes (Cloud) or filenames (OSS); look the | ||
| * asset up in the unified `inputAssetsByFilename` map and delegate the | ||
| * label resolution to {@link getAssetDisplayFilename} so this helper | ||
| * shares the fallback chain (`user_metadata.filename` → | ||
| * `metadata.filename` → `display_name` → `asset.name`) with the asset | ||
| * card / browser surfaces. Falls through to the raw input when no asset | ||
| * matches. | ||
| */ | ||
| export function getMediaDisplayName(name: string): string { | ||
| if (!isCloud) return name | ||
| return useAssetsStore().getInputName(name) | ||
| const asset = useAssetsStore().inputAssetsByFilename.get(name) | ||
| return asset ? getAssetDisplayFilename(asset) : name | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Subtle behavioural drift worth flagging in the PR description: the old
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and intended per BE-808. Both |
||
| } | ||
|
|
||
| export function useMissingMediaInteractions() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
name: 'sunset.png'is already inherited from...ossShape— the explicit re-assignment is a no-op and slightly muddies what the test is varying (thedisplay_namefield). Safe to drop.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done — dropped the redundant re-assignment; the fixture now only varies
display_name.