[feat] Add Civitai model upload wizard #6694
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
luke-mino-altherr
requested review from
a team,
KarryCharon and
shinshin86
as code owners
dosubot
bot
added
the
size:XL
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 11/20/2025, 04:26:33 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 11/20/2025, 04:38:07 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.13 MB (baseline 3.12 MB) • 🔴 +4.64 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 945 kB (baseline 925 kB) • 🔴 +19.6 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 7.97 kB (baseline 7.97 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 306 kB (baseline 306 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 141 kB (baseline 141 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 2.94 kB (baseline 2.94 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 3.87 MB (baseline 3.87 MB) • ⚪ 0 BBundles that do not match a named category
Status: 18 added / 18 removed |
Contributor
Author
luke-mino-altherr
left a comment
luke-mino-altherr
left a comment
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: [feat] Add Civitai model upload wizard (#6694)
Impact: 648 additions, 2 deletions across 10 files
Issue Distribution
- Critical: 1
- High: 3
- Medium: 4
- Low: 1
Category Breakdown
- Architecture: 2 issues
- Security: 1 issue
- Performance: 0 issues
- Code Quality: 6 issues
Key Findings
Architecture & Design
The PR implements a clean multi-step wizard pattern with good separation of concerns. However, there are some architectural concerns:
- Hard-coded defaults - The default model type 'loras' is set without validation that it exists in the available types
- Interface duplication - ModelMetadata interface is duplicated across 3 components, violating DRY principles
- Missing state cleanup - No reset logic when dialog closes, could lead to stale state if reopened
The composable pattern for useModelTypes is well-designed with singleton state and promise deduplication, following Vue 3 best practices.
Security Considerations
Critical Issue: No URL validation to enforce Civitai-only domains. The UI promises "Only links from https://civitai.com are supported" but there's no enforcement. Users could submit arbitrary URLs which could:
- Lead to SSRF vulnerabilities on the backend
- Allow malicious content downloads
- Bypass intended security boundaries
This should be addressed before merge by adding client-side URL validation.
Performance Impact
No significant performance concerns identified. The PR follows good practices:
- Lazy component loading via conditional rendering
- Singleton pattern in useModelTypes prevents duplicate API calls
- Minimal bundle size impact (~650 LOC across focused components)
Code Quality
Several quality issues need attention:
- Incomplete error handling - TODO comment indicates unfinished user feedback implementation
- Console.error usage - Production code shouldn't rely on console logging for user-facing errors
- Project guidelines violations:
- Hardcoded emoji (🎉) in template violates no-emoji guideline
- Hardcoded color
text-red-500instead of semantic theme tokens
- Missing loading states - UploadModelConfirmation doesn't handle async model type loading
Integration Points
The PR integrates well with existing systems:
- Uses established dialogStore pattern
- Follows i18n conventions with 26 new translation strings
- Properly extends assetService with new API methods
- Emits events for parent component coordination (upload-success)
Potential concerns:
- No backward compatibility issues (feature addition only)
- Extension compatibility not affected
- Assumes backend API endpoints exist (
/assets/metadata,/experiment/models)
Positive Observations
- Excellent component organization - Clear separation between URL input, confirmation, and progress steps
- Type safety - Strong TypeScript usage throughout with well-defined interfaces
- User experience - Multi-step wizard with clear progression and loading indicators
- Internationalization - All user-facing text properly externalized
- Service pattern - Clean API abstraction in assetService with proper error handling
- Composable design - useModelTypes follows Vue 3 composition API best practices
- Accessibility - Proper use of semantic HTML and ARIA-friendly components
References
Next Steps
Must Address (Before Merge)
- Fix critical security issue: Add Civitai URL validation
- Remove TODO comment: Implement proper error toast notification
- Fix hard-coded default: Make selectedModelType dynamic based on available types
Should Address (Before Merge)
- Extract ModelMetadata to shared types file
- Replace hardcoded emoji with i18n-based solution
- Use semantic color tokens instead of
text-red-500 - Add loading state handling in UploadModelConfirmation
Consider for Future
- Add component tests for the wizard workflow
- Add error state retry functionality
- Consider responsive sizing instead of fixed dimensions
- Add state reset on dialog close for better UX
This is a comprehensive automated review. The architectural design is sound and follows established patterns well. The critical security issue with URL validation must be addressed, along with completing the error handling implementation. Overall, this is a well-structured feature addition that integrates cleanly with the existing codebase.
|
The word upload implies you are uploading it to civitai, not from. Would calling it import or download make more sense? |
luke-mino-altherr
force-pushed
the
luke-mino-altherr/model-upload-modal
branch
from
e916eb3 to
d5f6d96
Compare
christian-byrne
previously approved these changes
Nov 20, 2025
Comment on lines
64
to
67
| onBack: () => void | ||
| onFetchMetadata: () => void | ||
| onUpload: () => void | ||
| onClose: () => void |
Contributor
There was a problem hiding this comment.
nit: the Vue-idiomatic way to do child->parent signals is via emit
<UploadFooter
:current-step="step"
...
@back="goBack"
@fetch-metadata="fetchMetadata"
@upload="uploadModel"
@close="closeDialog"
/>and in the component:
const emit = defineEmits<{
(e: 'back'): void
(e: 'fetch-metadata'): void
(e: 'upload'): void
(e: 'close'): void
}>()Source: https://vuejs.org/guide/essentials/component-basics#listening-to-events
| </div> | ||
|
|
||
| <div | ||
| class="flex flex-row items-start p-8 bg-node-component-surface rounded-lg" |
Contributor
There was a problem hiding this comment.
nit: node-component-surface seems like it might be the wrong token to use here.
Comment on lines
+8
to
+10
| <i | ||
| class="icon-[lucide--loader-circle] animate-spin text-6xl text-primary" | ||
| /> |
Contributor
There was a problem hiding this comment.
nit: we do have shared loading spinner component
| isLoading.value = true | ||
| error.value = null | ||
|
|
||
| fetchPromise = (async () => { |
Contributor
There was a problem hiding this comment.
nit: can simplify/improve with https://vueuse.org/core/useAsyncState/
| pathIndex: z.number() | ||
| }) | ||
|
|
||
| // Asset metadata from download URL |
Contributor
There was a problem hiding this comment.
question: What does this mean exactly?
Contributor
Author
There was a problem hiding this comment.
Can remove this comment since its not helpful. But the AssetMetadata provides information and backend validation around the given civitai URL so we can know if the URL is valid before attempting to download
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Wrap composable with createSharedComposable from VueUse to: - Fix HMR compatibility (state no longer lost on hot reload) - Remove import side effects from module-scoped variables - Create state lazily on first use while maintaining shared singleton behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ssages Extract navigation footer from UploadModelDialog into UploadModelFooter component. Add localized error messages for CivitAI validation error codes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
luke-mino-altherr
force-pushed
the
luke-mino-altherr/model-upload-modal
branch
2 times, most recently
from
c610d96 to
3f34c9e
Compare
luke-mino-altherr
force-pushed
the
luke-mino-altherr/model-upload-modal
branch
from
3f34c9e to
803beb2
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Nov 20, 2025
## Summary Adds a complete model upload workflow that allows users to import models from Civitai URLs directly into their library. ## Changes - **Multi-step wizard**: URL input → metadata confirmation → upload progress - **Components**: UploadModelDialog, UploadModelUrlInput, UploadModelConfirmation, UploadModelProgress, UploadModelDialogHeader - **API integration**: New assetService methods for metadata retrieval and URL-based uploads - **Model type management**: useModelTypes composable for fetching and formatting available model types - **UX improvements**: Optional validation bypass for UrlInput component - **Localization**: 26 new i18n strings for the upload workflow ## Review Focus - Error handling for failed uploads and metadata retrieval - Model type detection and selection logic - Dialog state management across multi-step workflow ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6694-feat-Add-Civitai-model-upload-wizard-2ab6d73d36508193b3b1dd67c7cc5a09) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude <[email protected]>
Member
|
@luke-mino-altherr Successfully backported to #6770 |
github-actions
bot
removed
the
needs-backport
christian-byrne
added
cloud/1.32
needs-backport
github-actions bot
pushed a commit
that referenced
this pull request
Nov 20, 2025
## Summary Adds a complete model upload workflow that allows users to import models from Civitai URLs directly into their library. ## Changes - **Multi-step wizard**: URL input → metadata confirmation → upload progress - **Components**: UploadModelDialog, UploadModelUrlInput, UploadModelConfirmation, UploadModelProgress, UploadModelDialogHeader - **API integration**: New assetService methods for metadata retrieval and URL-based uploads - **Model type management**: useModelTypes composable for fetching and formatting available model types - **UX improvements**: Optional validation bypass for UrlInput component - **Localization**: 26 new i18n strings for the upload workflow ## Review Focus - Error handling for failed uploads and metadata retrieval - Model type detection and selection logic - Dialog state management across multi-step workflow ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6694-feat-Add-Civitai-model-upload-wizard-2ab6d73d36508193b3b1dd67c7cc5a09) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude <[email protected]>
Member
|
@luke-mino-altherr Successfully backported to #6772 |
github-actions
bot
removed
the
needs-backport
christian-byrne
pushed a commit
that referenced
this pull request
Nov 20, 2025
Backport of #6694 to `cloud/1.32` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6772-backport-cloud-1-32-feat-Add-Civitai-model-upload-wizard-2b16d73d3650814dbeccc12c11427050) by [Unito](https://www.unito.io) Co-authored-by: Luke Mino-Altherr <[email protected]> Co-authored-by: Claude <[email protected]>
christian-byrne
pushed a commit
that referenced
this pull request
Nov 20, 2025
Backport of #6694 to `core/1.32` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6770-backport-core-1-32-feat-Add-Civitai-model-upload-wizard-2b16d73d365081f8a732f69713f95b61) by [Unito](https://www.unito.io) Co-authored-by: Luke Mino-Altherr <[email protected]> Co-authored-by: Claude <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Adds a complete model upload workflow that allows users to import models from Civitai URLs directly into their library.
Changes
Review Focus
┆Issue is synchronized with this Notion page by Unito