Skip to content

[feat] Add PostHog data-attr attributes to Upload Model flow #7173

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.

Sign up for GitHub

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

Merged
luke-mino-altherr merged 1 commit into from
Dec 5, 2025
Merged

[feat] Add PostHog data-attr attributes to Upload Model flow #7173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/platform/assets/components/AssetBrowserModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
type="accent"
size="md"
class="!h-10 [&>span]:hidden md:[&>span]:inline"
data-attr="upload-model-button"
:label="$t('assetBrowser.uploadModel')"
:on-click="showUploadDialog"
>
Expand Down
1 change: 1 addition & 0 deletions src/platform/assets/components/UploadModelConfirmation.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"
:options="modelTypes"
:disabled="isLoading"
data-attr="upload-model-step2-type-selector"
/>
<div class="flex items-center gap-2">
<i class="icon-[lucide--circle-question-mark]" />
Expand Down
15 changes: 12 additions & 3 deletions src/platform/assets/components/UploadModelFooter.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,20 @@
class="text-muted-foreground mr-auto underline flex items-center gap-2"
>
<i class="icon-[lucide--circle-question-mark]" />
<a href="#" target="_blank" class="text-muted-foreground">{{
$t('How do I find this?')
}}</a>
<a
href="#"
target="_blank"
class="text-muted-foreground"
data-attr="upload-model-step1-help-link"
>{{ $t('How do I find this?') }}</a
>
Comment on lines +8 to +14
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix the broken help link.

The help link has href="#" which is a placeholder that doesn't navigate anywhere. This provides a poor user experience, as clicking the "How do I find this?" link will not help users.

Consider one of the following:

  1. Link to actual documentation (e.g., CivitAI help page explaining how to find model URLs)
  2. Open a popover/tooltip with inline help text
  3. Remove the link if documentation isn't ready yet

If you need assistance implementing any of these solutions, I can help generate the code.

🤖 Prompt for AI Agents 
In src/platform/assets/components/UploadModelFooter.vue around lines 8 to 14 the
anchor uses href="#" which is a non-functional placeholder; replace it with a
real help action: ideally set href to the appropriate documentation URL (e.g.
the CivitAI help page) and keep target="_blank", add rel="noopener noreferrer"
and an aria-label for accessibility (preserve data-attr and styling), or if docs
aren't available convert the anchor into a button that opens an inline
popover/tooltip component with the help text or simply remove the link;
implement the chosen option consistently and ensure accessibility and security
attributes are included.

Copy link
Contributor Author

@luke-mino-altherr luke-mino-altherr Dec 5, 2025

Choose a reason for hiding this comment

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

Ah good reminder. need to implement this still

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.

@luke-mino-altherr, sounds good! Feel free to tag me if you'd like help implementing the help link—whether that's finding the right documentation URL, creating a popover component, or setting up the accessibility attributes. Happy to assist! 🐰

Copy link
Contributor Author

@luke-mino-altherr luke-mino-altherr Dec 5, 2025

Choose a reason for hiding this comment

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

I will implement (or remove) this in a separate pass

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.

@luke-mino-altherr, sounds good! Since you're planning to address this separately, would you like me to create a GitHub issue to track the help link implementation/removal? I can include a reference to this PR and discussion for context.

</span>
<TextButton
v-if="currentStep === 1"
:label="$t('g.cancel')"
type="transparent"
size="md"
data-attr="upload-model-step1-cancel-button"
:disabled="isFetchingMetadata || isUploading"
@click="emit('close')"
/>
Expand All @@ -22,6 +27,7 @@
:label="$t('g.back')"
type="transparent"
size="md"
:data-attr="`upload-model-step${currentStep}-back-button`"
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

Simplify the back button data-attr to a static string.

The back button only appears when currentStep !== 1 && currentStep !== 3, meaning it's exclusively shown on step 2. The dynamic template literal :data-attr="upload-model-step${currentStep}-back-button" will always resolve to "upload-model-step2-back-button", so the dynamic binding adds unnecessary complexity.

Apply this diff to simplify:

-      :data-attr="`upload-model-step${currentStep}-back-button`"
+      data-attr="upload-model-step2-back-button"
📝 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.

Suggested change
:data-attr="`upload-model-step${currentStep}-back-button`"
data-attr="upload-model-step2-back-button"
🤖 Prompt for AI Agents 
In src/platform/assets/components/UploadModelFooter.vue around line 30, the
data-attr for the back button is unnecessarily dynamic even though the button
only appears on step 2; replace the dynamic binding
:data-attr="`upload-model-step${currentStep}-back-button`" with a static
attribute data-attr="upload-model-step2-back-button" (remove the leading colon)
so the markup is simpler and always yields the same string when the button is
rendered.

:disabled="isFetchingMetadata || isUploading"
@click="emit('back')"
/>
Expand All @@ -32,6 +38,7 @@
:label="$t('g.continue')"
type="secondary"
size="md"
data-attr="upload-model-step1-continue-button"
:disabled="!canFetchMetadata || isFetchingMetadata"
@click="emit('fetchMetadata')"
>
Expand All @@ -47,6 +54,7 @@
:label="$t('assetBrowser.upload')"
type="secondary"
size="md"
data-attr="upload-model-step2-confirm-button"
:disabled="!canUploadModel || isUploading"
@click="emit('upload')"
>
Expand All @@ -62,6 +70,7 @@
:label="$t('assetBrowser.finish')"
type="secondary"
size="md"
data-attr="upload-model-step3-finish-button"
@click="emit('close')"
/>
</div>
Expand Down
1 change: 1 addition & 0 deletions src/platform/assets/components/UploadModelUrlInput.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
autofocus
:placeholder="$t('assetBrowser.civitaiLinkPlaceholder')"
class="w-full bg-secondary-background border-0 p-4"
data-attr="upload-model-step1-url-input"
/>
<p v-if="error" class="text-xs text-error">
{{ error }}
Expand Down