[feat] Add active jobs display to grid view #8209
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
viva-jinyi
added
enhancement
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new AssetsSidebarGridView component and ActiveJobCard (with tests); replaces per-item grid rendering in AssetsSidebarTab with the new grid component; moves active-job detection into shared isActiveJobState in Changes
Sequence Diagram(s)sequenceDiagram
participant Tab as AssetsSidebarTab
participant Grid as AssetsSidebarGridView
participant Store as useJobList
participant JobCard as ActiveJobCard
Tab->>Grid: pass props (assets, isSelected, assetType, showOutputCount, getOutputCount)
Grid->>Store: call/useJobList (compute active jobs via isActiveJobState)
Store-->>Grid: return activeJobItems
Grid->>JobCard: render ActiveJobCard for each active job (pass job)
JobCard-->>Grid: render spinner/progress/status
Grid-->>Tab: emit events (select-asset, context-menu, approach-end, zoom, output-count-click)
Tab-->>Tab: handle emitted events (selection, context menu, zoom)
Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/22/2026, 01:47:03 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
dosubot
bot
added
the
size:L
🎭 Playwright Tests:
|
coderabbitai
bot
requested review from
DrJKL,
KarryCharon,
Yorha4D,
benceruleanlu and
shinshin86
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.3 kB (baseline 22.3 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.02 MB (baseline 1.02 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 430 kB (baseline 430 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 8 added / 8 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.8 kB (baseline 2.8 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 32.8 kB (baseline 32.8 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 3.06 MB (baseline 3.05 MB) • 🔴 +7.03 kBStores, services, APIs, and repositories
Status: 7 added / 7 removed Utilities & Hooks — 18.1 kB (baseline 18.1 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 4 added / 4 removed Vendor & Third-Party — 10.4 MB (baseline 10.4 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 6.28 MB (baseline 6.28 MB) • ⚪ 0 BBundles that do not match a named category
Status: 22 added / 22 removed |
viva-jinyi
added
area:assets-sidebar
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 active jobs display to grid view (#8209)
Impact: 173 additions, 33 deletions across 4 files
Issue Distribution
- Critical: 0
- High: 1
- Medium: 4
- Low: 3
Category Breakdown
- Architecture: 2 issues
- Security: 0 issues
- Performance: 1 issues
- Code Quality: 5 issues
Key Findings
Architecture & Design
The refactoring to extract AssetsSidebarGridView follows good component separation patterns. However, there's code duplication in the isActiveJobState function between GridView and ListView components that should be addressed by extracting to a shared utility.
Security Considerations
No security issues identified. The components follow safe practices for UI rendering.
Performance Impact
Minor concern with removing overflow-y-auto from ListView which could affect scroll behavior with many jobs. Overall performance impact is minimal.
Integration Points
Good integration with existing useJobList composable and progress bar styling. ActiveJobCard properly integrates with the existing design system.
Positive Observations
- Excellent component extraction following established patterns
- Consistent use of existing composables (useProgressBarBackground, useJobList)
- Good separation of concerns between grid and list views
- Proper TypeScript typing throughout
- Consistent styling with design system
References
Next Steps
- Address the high-priority missing test issue before merge
- Consider extracting duplicate isActiveJobState function
- Review accessibility attributes for ActiveJobCard component
- Consider scroll behavior implications in ListView
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
benceruleanlu
requested changes
Jan 21, 2026
Member
benceruleanlu
left a comment
benceruleanlu
left a comment
There was a problem hiding this comment.
If there are several active jobs, they can overflow and hide all generated assets. Can that be fixed?
| <ActiveJobCard v-for="job in activeJobItems" :key="job.id" :job="job" /> | ||
| </div> | ||
|
|
||
| <!-- Assets Header --> |
Member
There was a problem hiding this comment.
nit: A followup is making this dependent on the dates, per the design
Comment on lines
82
to
85
| <div v-if="showLoadingState"> | ||
| <ProgressSpinner class="absolute left-1/2 w-[50px] -translate-x-1/2" /> | ||
| </div> | ||
| <div v-else-if="showEmptyState"> |
Member
There was a problem hiding this comment.
nit: when there are no generated assets, on the first active job, the grid view will not show the grids because of the logic in showLoadingState and showEmptyState
Member
There was a problem hiding this comment.
nit: design had slightly different ideas for the status text and progress bar designs, but that can be fixed later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/sidebar/tabs/assets/ActiveJobCard.test.ts`:
- Around line 54-87: Update ActiveJobCard.vue to mark the progress element with
role="progressbar" and set aria-valuenow to the numeric progress (when present)
and mark the spinner element with aria-hidden="true"; then update the tests in
ActiveJobCard.test.ts (the "displays progress bar when job is running with
progress", "hides progress bar when job is pending", and "displays spinner icon
in thumbnail area" cases) to use wrapper.find('[role="progressbar"]') and assert
the aria-valuenow value (or absence when pending) and to find the spinner via
'[aria-hidden="true"]' instead of relying on Tailwind classes; locate changes
via mountComponent and createJob helpers and the ActiveJobCard component.
Comment on lines
54
to
87
| it('displays progress bar when job is running with progress', () => { | ||
| const wrapper = mountComponent( | ||
| createJob({ state: 'running', progressTotalPercent: 65 }) | ||
| ) | ||
|
|
||
| const progressBar = wrapper.find('.bg-blue-500') | ||
| expect(progressBar.exists()).toBe(true) | ||
| expect(progressBar.attributes('style')).toContain('width: 65%') | ||
| }) | ||
|
|
||
| it('hides progress bar when job is pending', () => { | ||
| const wrapper = mountComponent( | ||
| createJob({ state: 'pending', progressTotalPercent: undefined }) | ||
| ) | ||
|
|
||
| const progressBar = wrapper.find('.bg-blue-500') | ||
| expect(progressBar.exists()).toBe(false) | ||
| }) | ||
|
|
||
| it('has proper accessibility attributes', () => { | ||
| const wrapper = mountComponent(createJob({ title: 'Generating...' })) | ||
|
|
||
| const container = wrapper.find('[role="status"]') | ||
| expect(container.exists()).toBe(true) | ||
| expect(container.attributes('aria-label')).toBe('Active job: Generating...') | ||
| }) | ||
|
|
||
| it('displays spinner icon in thumbnail area', () => { | ||
| const wrapper = mountComponent(createJob()) | ||
|
|
||
| const spinner = wrapper.find('.icon-\\[lucide--loader-circle\\]') | ||
| expect(spinner.exists()).toBe(true) | ||
| expect(spinner.classes()).toContain('animate-spin') | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the test file exists and verify the code snippet
find . -name "ActiveJobCard.test.ts" -type fRepository: Comfy-Org/ComfyUI_frontend
Length of output: 128
🏁 Script executed:
# Look for the component file
find . -name "ActiveJobCard.vue" -type fRepository: Comfy-Org/ComfyUI_frontend
Length of output: 124
🏁 Script executed:
# Check test content
if [ -f "src/components/sidebar/tabs/assets/ActiveJobCard.test.ts" ]; then
sed -n '54,87p' src/components/sidebar/tabs/assets/ActiveJobCard.test.ts
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1278
🏁 Script executed:
# Check component implementation
if [ -f "src/components/sidebar/tabs/assets/ActiveJobCard.vue" ]; then
wc -l src/components/sidebar/tabs/assets/ActiveJobCard.vue
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 125
🏁 Script executed:
# Read the component file to understand structure
cat -n src/components/sidebar/tabs/assets/ActiveJobCard.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2144
🏁 Script executed:
# Let's check what progressBarPrimaryClass actually contains
grep -n "progressBarPrimaryClass" src/components/sidebar/tabs/assets/ActiveJobCard.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 196
🏁 Script executed:
# Check the composable that provides the progress bar styling
find . -name "useProgressBarBackground*" -type fRepository: Comfy-Org/ComfyUI_frontend
Length of output: 115
🏁 Script executed:
# Read the composable
cat -n src/composables/useProgressBarBackground.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1776
🏁 Script executed:
# Let's check the full test file to understand the mount context and any setup
head -70 src/components/sidebar/tabs/assets/ActiveJobCard.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1994
🏁 Script executed:
# Check if there are other test files in the same directory for context
ls -la src/components/sidebar/tabs/assets/*.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 175
🏁 Script executed:
# Let's see how other components handle testing for progress/icons
find src/components -name "*.test.ts" -type f | head -5 | xargs grep -l "role=" 2>/dev/null | head -3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Let's check how other component tests in this repo handle progress bars or loading states
rg "role=\"progressbar\"" src/components --type ts -A 2 -B 2 | head -30Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Check if any existing tests use accessible properties for testing animations or icons
rg "aria-hidden" tests-ui/ src/components --type ts -A 1 -B 1 | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 119
🏁 Script executed:
# Check the directory structure for tests
find . -type d -name "*test*" | head -10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 496
🏁 Script executed:
# Look for other progress bar or loading spinner components
rg "bg-blue-500|bg-interface-panel-job-progress" src/components --type vue -A 2 -B 2 | head -30Repository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
# Check if the component renders the progress bar as a child element
sed -n '20,28p' src/components/sidebar/tabs/assets/ActiveJobCard.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 347
🏁 Script executed:
# Let's verify what the actual inner div of the progress bar looks like when rendered
# Check if progress bar inner element currently has any role or aria attributes
grep -A 3 'progressBarPrimaryClass' src/components/sidebar/tabs/assets/ActiveJobCard.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 349
🏁 Script executed:
# Check the exact structure again - looking at line 24-27
sed -n '24,27p' src/components/sidebar/tabs/assets/ActiveJobCard.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 193
🏁 Script executed:
# Let's check if other test files use data-testid for selectors
rg "data-testid" src/components --type ts -B 1 -A 1 | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1395
🏁 Script executed:
# Check the full test to see what the last test case looks like
tail -20 src/components/sidebar/tabs/assets/ActiveJobCard.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 762
🏁 Script executed:
# Verify the mock setup more carefully
sed -n '8,16p' src/components/sidebar/tabs/assets/ActiveJobCard.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 364
Replace class selectors with semantic ARIA attributes to avoid brittle style-dependent tests.
The tests select elements via Tailwind utility classes (bg-blue-500, animate-spin), which breaks when styling changes and conflicts with the "no style-dependent tests" guideline. Add role="progressbar" with aria-valuenow to the progress bar container and aria-hidden="true" to the spinner icon, then update tests to use these semantic properties instead.
🔧 Proposed test updates (ActiveJobCard.test.ts)
const progressBar = wrapper.find('.bg-blue-500')
expect(progressBar.exists()).toBe(true)
- expect(progressBar.attributes('style')).toContain('width: 65%')
+ expect(progressBar.attributes('aria-valuenow')).toBe('65')- const progressBar = wrapper.find('.bg-blue-500')
+ const progressBar = wrapper.find('[role="progressbar"]')
expect(progressBar.exists()).toBe(false)- const spinner = wrapper.find('.icon-\\[lucide--loader-circle\\]')
+ const spinner = wrapper.find('i[aria-hidden="true"]')
expect(spinner.exists()).toBe(true)
- expect(spinner.classes()).toContain('animate-spin')🧩 Required component updates (ActiveJobCard.vue)
<div
v-if="hasProgressPercent(progressPercent)"
+ role="progressbar"
+ :aria-valuenow="progressPercent"
+ aria-valuemin="0"
+ aria-valuemax="100"
class="flex-1 relative h-1 rounded-sm bg-secondary-background"
>
<i
+ aria-hidden="true"
class="icon-[lucide--loader-circle] size-8 animate-spin text-muted-foreground"
/>🤖 Prompt for AI Agents
In `@src/components/sidebar/tabs/assets/ActiveJobCard.test.ts` around lines 54 -
87, Update ActiveJobCard.vue to mark the progress element with
role="progressbar" and set aria-valuenow to the numeric progress (when present)
and mark the spinner element with aria-hidden="true"; then update the tests in
ActiveJobCard.test.ts (the "displays progress bar when job is running with
progress", "hides progress bar when job is pending", and "displays spinner icon
in thumbnail area" cases) to use wrapper.find('[role="progressbar"]') and assert
the aria-valuenow value (or absence when pending) and to find the spinner via
'[aria-hidden="true"]' instead of relying on Tailwind classes; locate changes
via mountComponent and createJob helpers and the ActiveJobCard component.
- Add max-h-[50%] and overflow-y-auto to prevent active jobs from hiding assets - Fix showLoadingState/showEmptyState to check activeJobsCount in grid view
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/sidebar/tabs/AssetsSidebarGridView.vue`:
- Around line 103-108: gridStyle is intended to be immutable; add a const
assertion to its declaration by changing the gridStyle object to use a
TypeScript const assertion (gridStyle = { ... } as const) so the compiler treats
its properties as readonly literal types; update any code that relies on wider
types if necessary to accommodate the narrower types from gridStyle being
readonly.
Comment on lines
+103
to
+108
| const gridStyle = { | ||
| display: 'grid', | ||
| gridTemplateColumns: 'repeat(auto-fill, minmax(200px, 1fr))', | ||
| padding: '0 0.5rem', | ||
| gap: '0.5rem' | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making gridStyle a const for clarity.
The gridStyle object is effectively immutable but declared with const already. This is fine, though using as const would make the immutability more explicit.
♻️ Optional: Add const assertion for immutability
-const gridStyle = {
+const gridStyle = {
display: 'grid',
gridTemplateColumns: 'repeat(auto-fill, minmax(200px, 1fr))',
padding: '0 0.5rem',
gap: '0.5rem'
-}
+} as const📝 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
| const gridStyle = { | |
| display: 'grid', | |
| gridTemplateColumns: 'repeat(auto-fill, minmax(200px, 1fr))', | |
| padding: '0 0.5rem', | |
| gap: '0.5rem' | |
| } | |
| const gridStyle = { | |
| display: 'grid', | |
| gridTemplateColumns: 'repeat(auto-fill, minmax(200px, 1fr))', | |
| padding: '0 0.5rem', | |
| gap: '0.5rem' | |
| } as const |
🤖 Prompt for AI Agents
In `@src/components/sidebar/tabs/AssetsSidebarGridView.vue` around lines 103 -
108, gridStyle is intended to be immutable; add a const assertion to its
declaration by changing the gridStyle object to use a TypeScript const assertion
(gridStyle = { ... } as const) so the compiler treats its properties as readonly
literal types; update any code that relies on wider types if necessary to
accommodate the narrower types from gridStyle being readonly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/sidebar/tabs/assets/ActiveJobCard.test.ts`:
- Around line 29-37: The test helper createJob is written as a function
expression; convert it to a function declaration to follow coding guidelines:
replace the const createJob = (overrides: Partial<JobListItem> = {}):
JobListItem => ({ ... }) with an equivalent function createJob(overrides:
Partial<JobListItem> = {}): JobListItem { return { id: 'test-job-1', title:
'Running...', meta: 'Step 5/10', state: 'running', progressTotalPercent: 50,
progressCurrentPercent: 75, ...overrides }; } and make the same change for the
other helper(s) in this test file that use arrow expressions so they become
function declarations with identical signatures and return values.
♻️ Duplicate comments (1)
src/components/sidebar/tabs/assets/ActiveJobCard.test.ts (1)
54-56: Tests rely on style-dependent class selectors.Using
.bg-blue-500and.animate-spinclass selectors makes tests brittle - they'll break if styling changes even when behavior is correct. This was flagged in a previous review suggesting ARIA attributes instead.Also applies to: 69-70, 76-78
Comment on lines
+29
to
+37
| const createJob = (overrides: Partial<JobListItem> = {}): JobListItem => ({ | ||
| id: 'test-job-1', | ||
| title: 'Running...', | ||
| meta: 'Step 5/10', | ||
| state: 'running', | ||
| progressTotalPercent: 50, | ||
| progressCurrentPercent: 75, | ||
| ...overrides | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer function declarations for helpers.
Per coding guidelines, prefer function declarations over function expressions for pure functions.
♻️ Proposed refactor
-const createJob = (overrides: Partial<JobListItem> = {}): JobListItem => ({
- id: 'test-job-1',
- title: 'Running...',
- meta: 'Step 5/10',
- state: 'running',
- progressTotalPercent: 50,
- progressCurrentPercent: 75,
- ...overrides
-})
+function createJob(overrides: Partial<JobListItem> = {}): JobListItem {
+ return {
+ id: 'test-job-1',
+ title: 'Running...',
+ meta: 'Step 5/10',
+ state: 'running',
+ progressTotalPercent: 50,
+ progressCurrentPercent: 75,
+ ...overrides
+ }
+}
-const mountComponent = (job: JobListItem) =>
- mount(ActiveJobCard, {
- props: { job },
- global: {
- plugins: [i18n]
- }
- })
+function mountComponent(job: JobListItem) {
+ return mount(ActiveJobCard, {
+ props: { job },
+ global: {
+ plugins: [i18n]
+ }
+ })
+}Also applies to: 39-45
🤖 Prompt for AI Agents
In `@src/components/sidebar/tabs/assets/ActiveJobCard.test.ts` around lines 29 -
37, The test helper createJob is written as a function expression; convert it to
a function declaration to follow coding guidelines: replace the const createJob
= (overrides: Partial<JobListItem> = {}): JobListItem => ({ ... }) with an
equivalent function createJob(overrides: Partial<JobListItem> = {}): JobListItem
{ return { id: 'test-job-1', title: 'Running...', meta: 'Step 5/10', state:
'running', progressTotalPercent: 50, progressCurrentPercent: 75, ...overrides };
} and make the same change for the other helper(s) in this test file that use
arrow expressions so they become function declarations with identical signatures
and return values.
|
Updating Playwright Expectations |
github-actions
bot
removed
the
New Browser Test Expectations
benceruleanlu
approved these changes
Jan 22, 2026
Member
benceruleanlu
left a comment
benceruleanlu
left a comment
There was a problem hiding this comment.
LGTM, thanks for the great work!
Member
There was a problem hiding this comment.
nit: IDK what CR said, but I personally would tighten selectors to not fail on non‑behavioral changes
This was referenced Jan 22, 2026
godwiniheuwa
pushed a commit
to godwiniheuwa/ComfyUI_frontend
that referenced
this pull request
Jan 22, 2026
## Summary Show active jobs in grid view matching the list view behavior, with refactored component structure. ## Changes - **ActiveJobCard**: New component for grid view job display with progress bar - **AssetsSidebarGridView**: Extracted grid view logic from AssetsSidebarTab (matching ListView pattern) - **Progress styling**: Use `useProgressBarBackground` composable for consistent progress bar styling - **Assets header**: Add "Generated/Imported assets" header in grid view
This was referenced Jan 23, 2026
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
Show active jobs in grid view matching the list view behavior, with refactored component structure.
Changes
useProgressBarBackgroundcomposable for consistent progress bar stylingFiles Changed
src/components/sidebar/tabs/assets/ActiveJobCard.vue(new)src/components/sidebar/tabs/AssetsSidebarGridView.vue(new)src/components/sidebar/tabs/AssetsSidebarTab.vue(refactored)screen-capture.webm
┆Issue is synchronized with this Notion page by Unito