Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/locales/en/main.json
Original file line number Diff line number Diff line change
Expand Up @@ -2113,6 +2113,8 @@
"uploadingModel": "Importing model...",
"uploadSuccess": "Model imported successfully!",
"uploadFailed": "Import failed",
"uploadModelHelpVideo": "Upload Model Help Video",
"uploadModelHowDoIFindThis": "How do I find this?",
"modelAssociatedWithLink": "The model associated with the link you provided:",
"modelTypeSelectorLabel": "What type of model is this?",
"modelTypeSelectorPlaceholder": "Select model type",
Expand Down
28 changes: 18 additions & 10 deletions src/platform/assets/components/UploadModelFooter.vue
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
<template>
<div class="flex justify-end gap-2 w-full">
<span
<button
v-if="currentStep === 1"
class="text-muted-foreground mr-auto underline flex items-center gap-2"
class="text-muted-foreground mr-auto underline flex items-center gap-2 cursor-pointer bg-transparent border-0 p-0"
data-attr="upload-model-step1-help-link"
@click="showVideoHelp = true"
>
<i class="icon-[lucide--circle-question-mark]" />
<a
href="#"
target="_blank"
class="text-muted-foreground"
data-attr="upload-model-step1-help-link"
>{{ $t('How do I find this?') }}</a
>
</span>
<span>{{ $t('assetBrowser.uploadModelHowDoIFindThis') }}</span>
</button>
Comment on lines +3 to +11
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 5, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Help button wiring and i18n usage look good; consider explicit button type

The step‑1 help trigger correctly:

  • Preserves data-attr="upload-model-step1-help-link" for tracking
  • Uses the new localized label assetBrowser.uploadModelHowDoIFindThis
  • Opens the video dialog via showVideoHelp = true

To avoid accidental form submission if this footer is ever rendered inside a <form>, it’s safer to mark this as a non‑submit button:

-    <button
+    <button
+      type="button"
       v-if="currentStep === 1"
       class="text-muted-foreground mr-auto underline flex items-center gap-2 cursor-pointer bg-transparent border-0 p-0"
       data-attr="upload-model-step1-help-link"
       @click="showVideoHelp = true"

Based on learnings, the move from a raw string to the localized key is the correct i18n pattern.

🤖 Prompt for AI Agents
In src/platform/assets/components/UploadModelFooter.vue around lines 3 to 11,
the help button is missing an explicit button type which can cause accidental
form submission when rendered inside a form; add type="button" to the <button>
element so it is explicitly a non-submit button, preserving all existing
attributes (v-if, classes, data-attr, @click) and localization.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with the IconTextButton, that way this will get migrated properly with the rest.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DrJKL, great point! Using IconTextButton will keep this consistent with the other buttons and handle the type="button" implicitly. Here's how to adapt it:

<IconTextButton
  v-if="currentStep === 1"
  :label="$t('assetBrowser.uploadModelHowDoIFindThis')"
  type="transparent"
  size="md"
  class="mr-auto underline text-muted-foreground"
  data-attr="upload-model-step1-help-link"
  @click="showVideoHelp = true"
>
  <template #icon>
    <i class="icon-[lucide--circle-question-mark]" />
  </template>
</IconTextButton>

This preserves:

  • The PostHog tracking attribute (data-attr)
  • The localized label
  • The icon slot pattern used elsewhere in the footer
  • The left alignment (mr-auto) and underline styling
  • The video dialog trigger

The transparent type should give you the link-like appearance while keeping the button accessible and consistent with the component architecture.

<TextButton
v-if="currentStep === 1"
:label="$t('g.cancel')"
Expand Down Expand Up @@ -73,12 +69,24 @@
data-attr="upload-model-step3-finish-button"
@click="emit('close')"
/>
<VideoHelpDialog
v-model="showVideoHelp"
video-url="https://media.comfy.org/compressed_768/civitai_howto.webm"
:aria-label="$t('assetBrowser.uploadModelHelpVideo')"
loop
:show-controls="false"
/>
</div>
</template>

<script setup lang="ts">
import { ref } from 'vue'
import IconTextButton from '@/components/button/IconTextButton.vue'
import TextButton from '@/components/button/TextButton.vue'
import VideoHelpDialog from '@/platform/assets/components/VideoHelpDialog.vue'
const showVideoHelp = ref(false)
defineProps<{
currentStep: number
Expand Down
96 changes: 96 additions & 0 deletions src/platform/assets/components/VideoHelpDialog.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
<template>
<Dialog
v-model:visible="isVisible"
modal
:closable="false"
:close-on-escape="false"
:dismissable-mask="true"
:pt="{
root: { class: 'video-help-dialog' },
header: { class: '!hidden' },
content: { class: '!p-0' },
mask: { class: '!bg-black/70' }
}"
:style="{ width: '90vw', maxWidth: '800px' }"
>
<div class="relative">
<IconButton
class="absolute top-4 right-6 z-10"
:aria-label="$t('g.close')"
@click="isVisible = false"
>
<i class="pi pi-times text-sm" />
</IconButton>
<video
:controls="showControls"
autoplay
muted
:loop="loop"
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional:

Suggested change
:loop="loop"
:loop

:aria-label="ariaLabel"
class="w-full rounded-lg"
:src="videoUrl"
>
{{ $t('g.videoFailedToLoad') }}
</video>
</div>
</Dialog>
</template>
Comment on lines +1 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Component behavior looks solid; consider making the default ariaLabel i18n-aware

Overall the dialog is well-structured: PrimeVue Dialog usage, modelValue/isVisible binding, capture-phase ESC handling with proper add/remove, and video autoplay/accessibility are all implemented correctly.

One minor improvement: the ariaLabel prop default is a hardcoded English string:

withDefaults(defineProps<...>(), {
  ariaLabel: 'Help video',
  loop: true,
  showControls: false
})

Given the i18n guidelines, it would be better to either:

  • Remove the default and force callers to pass a localized label, or
  • Provide a localized fallback, e.g. by falling back in the template:
-      <video
-        :controls="showControls"
-        autoplay
-        muted
-        :loop="loop"
-        :aria-label="ariaLabel"
+      <video
+        :controls="showControls"
+        autoplay
+        muted
+        :loop="loop"
+        :aria-label="ariaLabel || $t('assetBrowser.uploadModelHelpVideo')"
         class="w-full rounded-lg"
         :src="videoUrl"
       >

This ensures that any future use of the component without an explicit aria-label remains localized while keeping the current API.

Also applies to: 45-91

🤖 Prompt for AI Agents
In src/platform/assets/components/VideoHelpDialog.vue around lines 1-37 (and
also apply same change to lines 45-91), the ariaLabel prop currently defaults to
a hardcoded English string which breaks i18n; change the implementation so the
component does not use a hardcoded default but instead uses a localized fallback
at render time (or remove the default prop entirely and require callers to pass
it). Concretely: remove the hardcoded default in withDefaults or make ariaLabel
optional, and in the template compute ariaLabel via the $t() translator when the
prop is absent (e.g. use ariaLabel || $t('...')) so the rendered aria-label is
localized while preserving the existing prop API.


<script setup lang="ts">
import Dialog from 'primevue/dialog'
import { computed, onUnmounted, watch } from 'vue'
import IconButton from '@/components/button/IconButton.vue'
const props = withDefaults(
defineProps<{
modelValue: boolean
videoUrl: string
ariaLabel?: string
loop?: boolean
showControls?: boolean
Comment on lines +47 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

If any of these are always the same, I would remove them as props for now and just set them internally. YAGNI

}>(),
{
ariaLabel: 'Help video',
loop: true,
showControls: false
}
)
Comment on lines +45 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We don't need withDefaults with the version of Vue we're using
https://vuejs.org/guide/components/props.html#reactive-props-destructure
(modelValue intentionally omitted since that can be handled with defineModel)

Suggested change
const props = withDefaults(
defineProps<{
modelValue: boolean
videoUrl: string
ariaLabel?: string
loop?: boolean
showControls?: boolean
}>(),
{
ariaLabel: 'Help video',
loop: true,
showControls: false
}
)
const {
videoUrl,
ariaLabel = 'Help video',
loop = true,
showControls = false
} = defineProps<{
videoUrl: string
ariaLabel?: string
loop?: boolean
showControls?: boolean
}>()

const emit = defineEmits<{
'update:modelValue': [value: boolean]
}>()
Comment on lines +60 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

const isVisible = computed({
get: () => props.modelValue,
set: (value) => emit('update:modelValue', value)
})
const handleEscapeKey = (event: KeyboardEvent) => {
if (event.key === 'Escape' && isVisible.value) {
event.stopImmediatePropagation()
event.stopPropagation()
event.preventDefault()
isVisible.value = false
}
}
watch(
isVisible,
(visible) => {
if (visible) {
// Add listener with capture phase to intercept before parent dialogs
document.addEventListener('keydown', handleEscapeKey, { capture: true })
} else {
document.removeEventListener('keydown', handleEscapeKey, {
capture: true
})
}
},
{ immediate: true }
)
onUnmounted(() => {
document.removeEventListener('keydown', handleEscapeKey, { capture: true })
})
</script>
Loading