-
Notifications
You must be signed in to change notification settings - Fork 616
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 all commits
2184d25
e35bb25
2fd6725
f0299e5
283e741
edeeb2d
c52dcfc
1935b52
c7e83d8
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 |
|---|---|---|
|
|
@@ -420,6 +420,46 @@ describe('assetMetadataUtils', () => { | |
| }) | ||
| }) | ||
|
|
||
| describe('unified asset response shape (BE-808 RFC)', () => { | ||
| // Cloud asset: `asset.name` is a content hash; `display_name` carries | ||
| // the user-facing label. | ||
| const cloudShape: AssetItem = { | ||
| ...mockAsset, | ||
| id: 'cloud-asset-id', | ||
| name: 'blake3:abc1234567890def.png', | ||
| hash: 'blake3:abc1234567890def.png', | ||
| display_name: 'sunset.png' | ||
| } | ||
|
|
||
| // OSS asset: `asset.name` is already the filename; `display_name` is | ||
| // nullable per BE-1045 spec — clients fall back to `asset.name`. | ||
| const ossShape: AssetItem = { | ||
| ...mockAsset, | ||
| id: 'oss-asset-id', | ||
| name: 'sunset.png', | ||
| hash: null, | ||
| display_name: undefined | ||
|
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. nitpick: (non-blocking) ossShape sets hash: null but none of the three new tests exercise getAssetStoredFilename or getAssetUrlFilename, which are the only functions that consume hash. The field is inert here — either add a stored-filename assertion or drop it to avoid implying coverage that is not there.
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. nitpick: (non-blocking) ossShape sets hash: null but none of the three new tests exercise getAssetStoredFilename or getAssetUrlFilename, which are the only functions that consume hash. The field is inert here — either add a stored-filename assertion or drop it to avoid implying coverage that is not there. |
||
| } | ||
|
|
||
| it('renders the same label for the Cloud and OSS shapes via getAssetDisplayFilename', () => { | ||
| expect(getAssetDisplayFilename(cloudShape)).toBe('sunset.png') | ||
| expect(getAssetDisplayFilename(ossShape)).toBe('sunset.png') | ||
| }) | ||
|
|
||
| it('renders the same label via getAssetCardTitle', () => { | ||
| expect(getAssetCardTitle(cloudShape)).toBe('sunset.png') | ||
| expect(getAssetCardTitle(ossShape)).toBe('sunset.png') | ||
| }) | ||
|
|
||
| it('honours OSS-emitted display_name when present', () => { | ||
| const ossWithDisplayName: 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. nitpick: (non-blocking) "honours OSS-emitted display_name when present" exercises the same branch as the existing "falls back to display_name when filename metadata is absent" test at line 365 — identical code path, different fixture string. Per the project parsimony guideline this could be folded into the existing suite or dropped.
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. nitpick: (non-blocking) "honours OSS-emitted display_name when present" exercises the same branch as the existing "falls back to display_name when filename metadata is absent" test at line 365 — identical code path, different fixture string. Per the project parsimony guideline this could be folded into the existing suite or dropped.
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. nitpick: (non-blocking) "honours OSS-emitted display_name when present" exercises the same branch as the existing "falls back to display_name when filename metadata is absent" test at line 365 — identical code path, different fixture string. Per the project parsimony guideline this could be folded into the existing suite or dropped. |
||
| ...ossShape, | ||
|
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. Done — dropped the redundant re-assignment; the fixture now only varies |
||
| display_name: 'Curated Sunset' | ||
| } | ||
| expect(getAssetDisplayFilename(ossWithDisplayName)).toBe('Curated Sunset') | ||
| }) | ||
| }) | ||
|
|
||
| describe('getAssetMetadataDimensions', () => { | ||
| it('returns dimensions when width/height are positive integers', () => { | ||
| const asset = { ...mockAsset, metadata: { width: 1024, height: 768 } } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -189,13 +189,9 @@ export function getAssetStoredFilename(asset: AssetItem): string { | |
| } | ||
|
|
||
| /** | ||
| * Gets the human-readable filename to render in UI surfaces. | ||
| * 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}. | ||
| * Human-readable filename for UI labels. | ||
| * Fallback: user_metadata.filename → metadata.filename → display_name → 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. nitpick: (non-blocking) every other export in this file opens with an imperative verb (Gets the..., Extracts..., Resolves...) — the new noun-phrase opening "Human-readable filename for UI labels." breaks that consistency.
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. nitpick: (non-blocking) every other export in this file opens with an imperative verb (Gets the..., Extracts..., Resolves...) — the new noun-phrase opening "Human-readable filename for UI labels." breaks that consistency.
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. nitpick: (non-blocking) every other export in this file opens with an imperative verb (Gets the..., Extracts..., Resolves...) — the new noun-phrase opening "Human-readable filename for UI labels." breaks that consistency. |
||
| * 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
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. suggestion: (non-blocking) the shortened docstring drops "Use this helper for labels/titles only" — keeping that positive constraint alongside the existing cross-reference to getAssetFilename would make the display-vs-identifier distinction clear without needing to follow the link.
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. suggestion: (non-blocking) the shortened docstring drops "Use this helper for labels/titles only" — keeping that positive constraint alongside the existing cross-reference to getAssetFilename would make the display-vs-identifier distinction clear without needing to follow the link.
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. suggestion: (non-blocking) the shortened docstring drops "Use this helper for labels/titles only" — keeping that positive constraint alongside the existing cross-reference to getAssetFilename would make the display-vs-identifier distinction clear without needing to follow the link. |
||
| export function getAssetDisplayFilename(asset: AssetItem): string { | ||
| return ( | ||
|
|
@@ -208,7 +204,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') | ||
|
|
||
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.
nitpick: (non-blocking) ossShape sets hash: null but none of the three new tests exercise getAssetStoredFilename or getAssetUrlFilename, which are the only functions that consume hash. The field is inert here — either add a stored-filename assertion or drop it to avoid implying coverage that is not there.