-
Notifications
You must be signed in to change notification settings - Fork 491
Add job output preview helper #8293
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.
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
Conversation
…on-assets # Conflicts: # src/components/TopMenuSection.test.ts
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
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/24/2026, 04:25:25 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a reusable helper to derive previewable ResultItemImpl outputs from a JobDetail, centralizing output flattening/filtering for upcoming features.
Changes:
- Added
getPreviewableOutputsFromJobDetail(jobDetail)to flatten job outputs, attachnodeId/mediaType, and filter to previewable items. - Added unit tests covering empty inputs, filtering (animated/text/unknown), and mapping of metadata.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/services/jobOutputCache.ts | Introduces helper to flatten JobDetail.outputs into previewable ResultItemImpl[]. |
| src/services/jobOutputCache.test.ts | Adds unit tests validating filtering and metadata mapping behavior. |
| function getPreviewableOutputs(outputs?: TaskOutput): ResultItemImpl[] { | ||
| if (!outputs) return [] | ||
| const resultItems = Object.entries(outputs).flatMap(([nodeId, nodeOutputs]) => | ||
| Object.entries(nodeOutputs) | ||
| .filter(([mediaType, items]) => mediaType !== 'animated' && items) | ||
| .flatMap(([mediaType, items]) => { | ||
| if (!Array.isArray(items)) { | ||
| return [] | ||
| } | ||
|
|
||
| return items.filter(isResultItem).map( | ||
| (item) => | ||
| new ResultItemImpl({ | ||
| ...item, | ||
| nodeId, | ||
| mediaType | ||
| }) | ||
| ) | ||
| }) | ||
| ) | ||
|
|
||
| return ResultItemImpl.filterPreviewable(resultItems) | ||
| } |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flattening/mapping logic here duplicates TaskItemImpl.calculateFlatOutputs() (src/stores/queueStore.ts:251-267) with slightly different filtering behavior. To avoid future divergence, consider extracting a shared helper (e.g., flattenTaskOutputs) that both places call, and keep the previewable filtering (filterPreviewable) separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's close but not identical, mainly because of the input differences that leads to differences in the filtering logic required. I would avoid changing this for now, maybe extracting a pure shared helper/util later.
src/services/jobOutputCache.ts
Outdated
| } | ||
|
|
||
| function isResultItem(item: unknown): item is ResultItem { | ||
| return typeof item === 'object' && item !== null |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isResultItem is an unsound type guard: it returns true for any non-null object (including arrays), but claims the value is a ResultItem. This can mask bad data and potentially lead to runtime errors inside ResultItemImpl (e.g., non-string filename). Tighten the guard (e.g., exclude arrays and validate that filename/subfolder/type are strings when present), or replace it with a schema-based safeParse if available.
| return typeof item === 'object' && item !== null | |
| if (typeof item !== 'object' || item === null || Array.isArray(item)) { | |
| return false | |
| } | |
| const candidate = item as Record<string, unknown> | |
| if ('filename' in candidate && typeof candidate.filename !== 'string') { | |
| return false | |
| } | |
| if ('subfolder' in candidate && typeof candidate.subfolder !== 'string') { | |
| return false | |
| } | |
| if ('type' in candidate && typeof candidate.type !== 'string') { | |
| return false | |
| } | |
| return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tightened it
viva-jinyi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The base branch was changed.
|
Replaced by #8283 |
Add helper to derive previewable outputs from job detail.
Summary
Centralize job detail output parsing so previewable outputs can be reused by upcoming asset stack/folder logic.
Changes
getPreviewableOutputsFromJobDetailand unit tests that cover filtering and mapping behavior.Review Focus
Screenshots (if applicable)
N/A
┆Issue is synchronized with this Notion page by Unito