feat: ext 6788 asset sidebar location constants [EXT-6786] #2338
Conversation
WalkthroughAdded an Asset API surface: new asset types, createAsset implementation, a LOCATION_ASSET_SIDEBAR location, ConnectMessage asset payload, and wiring of a makeAssetAPI producer into the location-to-producer map to expose asset methods in the asset sidebar. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Sidebar as Asset Sidebar (LOCATION_ASSET_SIDEBAR)
participant Prod as makeAssetAPI
participant AssetAPI as createAsset / AssetAPI
participant Channel
participant Host
App->>Sidebar: connect({ asset, ... })
Sidebar->>Prod: produce API with channel + { asset }
Prod->>AssetAPI: createAsset(channel, asset)
Note over AssetAPI: init sys/metadata, register handlers
App->>AssetAPI: getSys / getMetadata
AssetAPI-->>App: current values
App->>AssetAPI: publish(options)
AssetAPI->>Channel: call('callAssetMethod','publish',options)
Channel->>Host: forward call
Host-->>Channel: result
Channel-->>AssetAPI: sys/metadata update events
AssetAPI-->>App: onSysChanged / onMetadataChanged callbacks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Comment |
- Add LOCATION_ASSET_SIDEBAR to locations.ts - Update Locations interface in api.types.ts - Part of EXT-6786: Asset Sidebar App Framework Location
- Added createAsset import and makeAssetAPI function to handle asset-related messages. - Updated LOCATION_TO_API_PRODUCERS to include asset sidebar API producers. - Enhanced ConnectMessage interface to support asset metadata and system information. - Introduced AssetSys interface for asset type representation. This change supports the new asset sidebar functionality.
tylerwashington888
force-pushed
the
feature/EXT-6788-asset-sidebar-location-constants
branch
from
ab5c077 to
5c43347
Compare
tylerwashington888
changed the title
tylerwashington888
changed the title
- Removed the optional metadata property from the AssetAPI interface, marking it as deprecated. - Updated createAsset function to eliminate the metadata parameter, streamlining asset creation. This change enhances the clarity and usability of the asset API.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/types/api.types.ts (1)
415-418: ConnectMessage shape likely needs a union for asset vs entry locations.Requiring
entrywhile adding optionalassetleads to an impossible shape for asset-sidebar. Consider a location-driven union to improve type safety and DX.You can introduce a union like:
// Outside changed lines: illustrative interface ConnectMessageBase { /* existing shared props */ } interface EntryConnectMessage extends ConnectMessageBase { entry: { sys: EntrySys; metadata?: Metadata } asset?: never } interface AssetConnectMessage extends ConnectMessageBase { asset: { sys: AssetSys; metadata?: Metadata } entry?: never } export type ConnectMessage = EntryConnectMessage | AssetConnectMessage /* | other variants as needed */Optionally, consider adding
assettoIdsAPIfor parity with entry locations.lib/api.ts (1)
156-160: Add an explicit guard for missingasset(parity with Field API error).
createAssetthrows, but adding a guard here improves error clarity and consistency.Apply this diff:
function makeAssetAPI(channel: Channel, { asset }: ConnectMessage) { - return { - asset: createAsset(channel, asset), - } + if (!asset) { + throw new Error('AssetAPI called for location without "asset" property defined.') + } + return { + asset: createAsset(channel, asset), + } }lib/asset.ts (1)
20-28: Optional: namespace channel events to avoid future collisions.If entry and asset APIs coexist in a location later,
sysChanged/metadataChangedmay collide. ConsiderassetSysChanged/assetMetadataChanged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/api.ts(3 hunks)lib/asset.ts(1 hunks)lib/locations.ts(1 hunks)lib/types/api.types.ts(3 hunks)lib/types/asset.types.ts(1 hunks)lib/types/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
lib/types/asset.types.ts (2)
lib/types/utils.ts (1)
AssetSys(73-75)lib/types/index.ts (1)
Metadata(61-61)
lib/asset.ts (5)
lib/channel.ts (1)
Channel(26-86)lib/types/api.types.ts (1)
ConnectMessage(397-424)lib/types/asset.types.ts (1)
AssetAPI(4-22)lib/types/utils.ts (1)
AssetSys(73-75)lib/signal.ts (1)
MemoizedSignal(24-57)
lib/types/api.types.ts (1)
lib/types/utils.ts (1)
AssetSys(73-75)
lib/types/utils.ts (1)
lib/types/index.ts (1)
ContentEntitySys(97-97)
lib/api.ts (3)
lib/channel.ts (1)
Channel(26-86)lib/types/api.types.ts (1)
ConnectMessage(397-424)lib/asset.ts (1)
createAsset(7-54)
🔇 Additional comments (6)
lib/types/api.types.ts (2)
17-17: Import looks good.The new
AssetSysimport is correct and aligns with the added asset shape.
389-389: New location constant LGTM.
LOCATION_ASSET_SIDEBAR: 'asset-sidebar'is consistent and will help route the correct API surface.lib/locations.ts (1)
7-7: Location mapping addition LGTM.
LOCATION_ASSET_SIDEBARis correctly added to the exported map.lib/api.ts (2)
5-5: Import addition LGTM.
createAssetimport correctly wires the new API producer.
42-42: Producer wiring LGTM.Mapping
LOCATION_ASSET_SIDEBARto[makeSharedAPI, makeAssetAPI, makeWindowAPI]is correct and avoids entry-specific producers.lib/types/asset.types.ts (1)
4-22: AssetAPI surface LGTM.Methods and callback types are coherent; deprecating the
metadataproperty while providing getters/listeners is a good balance.
| onMetadataChanged(handler: VoidFunction) { | ||
| return metadataChanged.attach(handler) | ||
| }, |
There was a problem hiding this comment.
Type mismatch: onMetadataChanged callback should receive metadata.
The signature is VoidFunction but listeners are invoked with metadata. Align with AssetAPI type.
Apply this diff:
- onMetadataChanged(handler: VoidFunction) {
+ onMetadataChanged(handler: (metadata?: Metadata) => void) {
return metadataChanged.attach(handler)
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onMetadataChanged(handler: VoidFunction) { | |
| return metadataChanged.attach(handler) | |
| }, | |
| onMetadataChanged(handler: (metadata?: Metadata) => void) { | |
| return metadataChanged.attach(handler) | |
| }, |
🤖 Prompt for AI Agents
lib/asset.ts around lines 50 to 52: the onMetadataChanged handler is currently
typed as VoidFunction but listeners are invoked with metadata; change the
handler signature to accept the metadata parameter matching AssetAPI (e.g.
handler: (metadata: AssetMetadata) => void or the exact type from AssetAPI),
update the function signature and return type accordingly, and import or
reference the correct AssetAPI/Metadata type so attached listeners receive the
typed metadata.
There was a problem hiding this comment.
the identical entry api method uses the same args so im going to keep this the same
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for onMetadataChanged method in entry-related files to verify the user's claim about consistency
ast-grep --pattern 'onMetadataChanged($_) {
$$$
}'
# Also search with rg to find any onMetadataChanged implementations
rg -nC3 "onMetadataChanged" --type=ts
Length of output: 1234
@tylerwashington888 You're absolutely right! I can see that the entry API implementation in lib/entry.ts line 77 uses the exact same pattern:
onMetadataChanged(handler: VoidFunction) {
return metadataChanged.attach(handler)
}Both implementations are consistent with each other, even though they differ from their respective type definitions (which correctly specify (metadata?: Metadata) => void). I understand the rationale for maintaining consistency across the codebase. Thanks for the clarification!
✏ Learnings added
Learnt from: tylerwashington888
PR: contentful/ui-extensions-sdk#2338
File: lib/asset.ts:49-51
Timestamp: 2025-09-24T16:31:58.939Z
Learning: Both asset API and entry API implementations use VoidFunction for onMetadataChanged handler parameter, maintaining consistency across the codebase despite type definitions specifying (metadata?: Metadata) => void.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| export interface AssetSys extends ContentEntitySys { | ||
| type: 'Asset' | ||
| } |
There was a problem hiding this comment.
AssetSys should not inherit contentType (assets don’t have one).
Current inheritance from ContentEntitySys incorrectly forces a contentType link on assets. Define AssetSys by omitting contentType.
Apply this diff:
-export interface AssetSys extends ContentEntitySys {
- type: 'Asset'
-}
+export type AssetSys = Omit<ContentEntitySys, 'contentType'> & {
+ type: 'Asset'
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface AssetSys extends ContentEntitySys { | |
| type: 'Asset' | |
| } | |
| export type AssetSys = Omit<ContentEntitySys, 'contentType'> & { | |
| type: 'Asset' | |
| } |
🤖 Prompt for AI Agents
In lib/types/utils.ts around lines 73 to 75, AssetSys currently inherits
ContentEntitySys which forces a contentType link that assets don't have; change
AssetSys to extend ContentEntitySys with contentType omitted (e.g., use
Omit<ContentEntitySys, 'contentType'>) and keep the type literal 'Asset' so the
interface no longer requires a contentType property.
There was a problem hiding this comment.
the identical entry interface uses the same structure so im going to keep this the same
There was a problem hiding this comment.
@tylerwashington888 Thanks for the clarification! You're absolutely right that maintaining consistency between EntrySys and AssetSys by having them both extend ContentEntitySys is a valid design choice. I understand you want to keep the interface structure aligned across entity types.
✏ Learnings added
Learnt from: tylerwashington888
PR: contentful/ui-extensions-sdk#2338
File: lib/types/utils.ts:73-0
Timestamp: 2025-09-24T16:32:36.609Z
Learning: In the Contentful UI Extensions SDK, both EntrySys and AssetSys interfaces extend ContentEntitySys to maintain consistency, even though assets may not typically use all inherited properties like contentType. This is an intentional design decision for API consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
lewisjcs
left a comment
lewisjcs
left a comment
There was a problem hiding this comment.
I would look into what CodeRabbit found to double-check, but I don't see anything else.
tylerwashington888
deleted the
feature/EXT-6788-asset-sidebar-location-constants
branch
# [4.45.0](v4.44.1...v4.45.0) (2025-09-24) ### Features * ext 6788 asset sidebar location constants [EXT-6786] ([#2338](#2338)) ([87ab2b1](87ab2b1))
|
🎉 This PR is included in version 4.45.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Add Asset Sidebar App Framework Location Support
Purpose
This PR implements the foundational support for the new asset-sidebar App Framework location, enabling apps to be installed and rendered in the asset editing sidebar. This addresses critical customer needs for asset-centric workflows including AI tagging, rights management, and publishing automation.
Background
As part of the asset-sidebar epic (EXT-6786, EXT-6788, EXT-6790, EXT-6793), this implementation provides the core SDK infrastructure needed to support apps in the asset editing context, similar to how entry-sidebar works for entries.
Changes
��️ Location Constants
lib/locations.ts: Added LOCATION_ASSET_SIDEBAR: 'asset-sidebar' to the locations object
lib/types/api.types.ts: Updated Locations interface to include the new location type
🔧 Asset API Implementation
lib/types/asset.types.ts: Created AssetAPI interface with asset-specific operations:
getSys(), publish(), unpublish(), save()
onSysChanged(), getMetadata(), onMetadataChanged()
lib/asset.ts: Implemented createAsset() function following same patterns as createEntry()
lib/types/utils.ts: Added AssetSys interface extending ContentEntitySys
🔌 API Producer Integration
lib/api.ts:
Added makeAssetAPI() function to provide asset context to apps
Added [locations.LOCATION_ASSET_SIDEBAR]: [makeSharedAPI, makeAssetAPI, makeWindowAPI] to LOCATION_TO_API_PRODUCERS
Imported and integrated createAsset() function
📡 Data Flow Support
lib/types/api.types.ts: Updated ConnectMessage interface to include optional asset property:
Impact
✅ Apps can now specify asset-sidebar as a supported location in their app definitions
✅ Apps running in asset-sidebar location receive sdk.asset API with full asset operations
✅ TypeScript compilation passes with new location and API types
✅ Foundation established for UI integration and widget rendering
✅ No breaking changes to existing functionality
Type: Feature
Scope: UI Extensions SDK
Breaking Change: No
Related Tickets: EXT-6786, EXT-6788, EXT-6790, EXT-6793
Summary by CodeRabbit
New Features
Types