Skip to content

Conversation

arjansingh
Copy link
Contributor

@arjansingh arjansingh commented Sep 18, 2025

Summary

Add asset browser dialog integration for combo widgets with full animation support and proper state management.

(Thank you Claude from saving me me from merge conflict hell on this one.)

Changes

  • Widget integration: combo widgets now use AssetBrowserModal for eligible asset types
  • Dialog animations: added animateHide() for smooth close transitions
  • Async operations: proper sequencing of widget updates and dialog animations
  • Service layer: added getAssetsForNodeType() and getAssetDetails() methods
  • Type safety: comprehensive TypeScript types and error handling
  • Test coverage: unit tests for all new functionality
  • Bonus: fixed the hardcoded labels in AssetFilterBar

Widget behavior:

  • Shows asset browser button for eligible widgets when asset API enabled
  • Handles asset selection with proper callback sequencing
  • Maintains widget value updates and litegraph notification

Review Focus

I will call out some stuff inline.

Screenshots

2025.09.17.AssetBrowserMVP.mov

┆Issue is synchronized with this Notion page by Unito

Add asset browser dialog integration for combo widgets with full animation support and proper state management.

Key changes:
- Widget integration: combo widgets now use AssetBrowserModal for eligible asset types
- Dialog animations: added animateHide() for smooth close transitions
- Async operations: proper sequencing of widget updates and dialog animations
- Service layer: added getAssetsForNodeType() and getAssetDetails() methods
- Type safety: comprehensive TypeScript types and error handling
- Test coverage: unit tests for all new functionality

Widget behavior:
- Shows asset browser button for eligible widgets when asset API enabled
- Handles asset selection with proper callback sequencing
- Maintains widget value updates and litegraph notification

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
"lint": "eslint src --cache",
"lint:fix": "eslint src --cache --fix",
"lint:unstaged": "git diff --name-only HEAD | grep -E '\\.(js|ts|vue|mts)$' | xargs -r eslint --cache",
"lint:unstaged:fix": "git diff --name-only HEAD | grep -E '\\.(js|ts|vue|mts)$' | xargs -r eslint --cache --fix",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just helper functions for myself.

Copy link

github-actions bot commented Sep 18, 2025

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 09/19/2025, 08:06:53 PM UTC

📈 Summary

  • Total Tests: 449
  • Passed: 419 ✅
  • Failed: 0
  • Flaky: 1 ⚠️
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 412 / ❌ 0 / ⚠️ 1 / ⏭️ 29
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

@arjansingh arjansingh marked this pull request as ready for review September 18, 2025 02:57
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Sep 18, 2025
Copy link

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

  • ESLint auto-fixes
  • Prettier formatting

@DrJKL DrJKL self-assigned this Sep 18, 2025
DrJKL
DrJKL previously approved these changes Sep 18, 2025
it('should handle missing user_metadata.filename as error', async () => {
const consoleErrorSpy = vi
.spyOn(console, 'error')
.mockImplementation(() => {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could keep the default behavior and set silent to passed-only if the error message in the console would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit?

@DrJKL DrJKL removed their assignment Sep 18, 2025
// Handle asset selection and emit to parent
const handleAssetSelectAndEmit = (asset: AssetDisplayItem) => {
selectAsset(asset) // This logs the selection for dev mode
const handleAssetSelectAndEmit = async (asset: AssetDisplayItem) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: (This does not need to be done in this PR). We can consider moving the selection to the onBeforeUnmount of the modal. One benefit is that right now, we are doing a blocking schema validation and potentially network request on each selection which may cause sluggishness during UI interactions (compared with deferring that to a single time when closing dialog).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look at this in follow up.

Copy link

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

  • ESLint auto-fixes
  • Prettier formatting

'hover:transform hover:-translate-y-0.5 hover:shadow-lg hover:shadow-black/10 hover:border-gray-400',
'dark-theme:hover:shadow-lg dark-theme:hover:shadow-black/30 dark-theme:hover:border-charcoal-700',
'focus:outline-none focus:ring-2 focus:ring-blue-500 dark-theme:focus:ring-blue-400'
'focus:outline-none focus:transform focus:-translate-y-0.5 focus:shadow-lg focus:shadow-black/10 dark-theme:focus:shadow-black/30'
Copy link
Contributor Author

@arjansingh arjansingh Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more in line with current design system.

Comment on lines +46 to +55
const assets: AssetItem[] = await assetService
.getAssetsForNodeType(props.nodeType)
.catch((error) => {
console.error(
'Failed to fetch assets for node type:',
props.nodeType,
error
)
return []
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might cause some sluggishness because the user will do the action to open the dialog -> we must wait for the request to resolve -> then dialog opens.

A possible good alternative that we use is something like:

<template>
  <ProgressSpinner v-if="isLoading" />
  <div v-else-if="error">Failed to load assets</div>
  <AssetGrid v-else :assets="assets" />
</template>

<script setup lang="ts">
import { useAsyncState } from '@vueuse/core'
import ProgressSpinner from 'primevue/progressspinner'
import type { AssetItem } from '@/platform/assets/schemas/assetSchema'
import { assetService } from '@/platform/assets/services/assetService'

const { nodeType } = defineProps<{ nodeType: string }>()

const {
  state: assets,
  isLoading,
  error,
} = useAsyncState<AssetItem[]>(
  () => assetService.getAssetsForNodeType(nodeType),
  [],
  { immediate: true }
)
</script>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can also use the error component we often use:

<template>
  <div>
    <ProgressSpinner v-if="isLoading" />

    <NoResultsPlaceholder
      v-else-if="error"
      icon="pi pi-exclamation-triangle"
      title="Failed to load assets"
      message="There was a problem fetching assets for this node type."
      buttonLabel="Retry"
      @action="execute"
    />

    <AssetGrid v-else :assets="assets" />
  </div>
</template>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will get this in a future PR. Good point.

@christian-byrne christian-byrne merged commit fd12591 into main Sep 20, 2025
21 checks passed
@christian-byrne christian-byrne deleted the feat/asset-browser-modal-integration branch September 20, 2025 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:assets size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants