feat: wire essentials_category for Essentials tab display#9091
Open
christian-byrne wants to merge 3 commits intomainfrom
Open
feat: wire essentials_category for Essentials tab display#9091christian-byrne wants to merge 3 commits intomainfrom
christian-byrne wants to merge 3 commits intomainfrom
Conversation
dosubot
bot
added
the
size:L
|
Related Documentation Checked 0 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
🎨 Storybook: ✅ Built — View Storybook |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduce a centralized essentials node constants module, re-export toolkit sets, add an optional Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
🎭 Playwright: ✅ 543 passed, 0 failed · 7 flaky📊 Browser Reports
|
📦 Bundle: 4.41 MB gzip 🔴 +329 BDetailsSummary
Category Glance App Entry Points — 17.9 kB (baseline 17.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 970 kB (baseline 970 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.1 kB (baseline 72.1 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 435 kB (baseline 435 kB) • 🔴 +1 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 736 B (baseline 736 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 47.1 kB (baseline 47.1 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.54 MB (baseline 2.54 MB) • 🔴 +459 BStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 55.5 kB (baseline 55.5 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 11 added / 11 removed Vendor & Third-Party — 8.84 MB (baseline 8.84 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.7 MB (baseline 7.7 MB) • ⚪ 0 BBundles that do not match a named category
Status: 51 added / 51 removed |
dante01yoon
previously approved these changes
Feb 22, 2026
christian-byrne
added
the
New Browser Test Expectations
|
Updating Playwright Expectations |
github-actions
bot
removed
the
New Browser Test Expectations
Member
|
Sorry about the late review, taking a look now |
Member
benceruleanlu
left a comment
benceruleanlu
left a comment
There was a problem hiding this comment.
- indexOf is case sensitive, the backend PR adds title cased categories, while the frontend expects lowercased, this will cause mismatches
- I don't know if snapshot changes are expected
- Add essentialsNodes.ts as single source of truth for node categorization and ordering, consolidating three separate lists - Pass essentials_category through subgraphStore from blueprints and global subgraph data to node definitions - Add essentials_category to SubgraphDefinitionBase schema - Refactor toolkitNodes.ts, nodeSource.ts, nodeOrganizationService.ts to import from the centralized constants - Add tests for constants integrity and subgraph passthrough Fixes COM-15221 Amp-Thread-ID: https://ampcode.com/threads/T-019c83de-f7ab-7779-a451-0ba5940b56a9
christian-byrne
force-pushed
the
toolkit/wire-essentials-categorization
branch
from
70ccad6 to
13e40d0
Compare
Contributor
Author
|
Addressed review feedback:
Also rebased on latest main. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/constants/essentialsNodes.ts (1)
81-95: Preserve category union type in derived maps.Line 81 and Line 93 currently widen values to
string; that losesEssentialsCategorysafety for consumers.♻️ Proposed type-safety refinement
-export const ESSENTIALS_CATEGORY_MAP: Record<string, string> = +export const ESSENTIALS_CATEGORY_MAP: Record<string, EssentialsCategory> = Object.fromEntries( Object.entries(ESSENTIALS_NODES).flatMap(([category, nodes]) => nodes.map((node) => [node, category]) ) ) -export const ESSENTIALS_CATEGORY_CANONICAL: ReadonlyMap<string, string> = +export const ESSENTIALS_CATEGORY_CANONICAL: + ReadonlyMap<string, EssentialsCategory> = new Map(ESSENTIALS_CATEGORIES.map((c) => [c.toLowerCase(), c]))As per coding guidelines
src/**/*.ts: Use TypeScript for type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/constants/essentialsNodes.ts` around lines 81 - 95, The derived maps currently widen their value types to string; change their declarations and/or casts to preserve the EssentialsCategory union. For ESSENTIALS_CATEGORY_MAP, replace Record<string, string> with Record<string, EssentialsCategory> (or assert the result: Object.fromEntries(...) as Record<string, EssentialsCategory>) and ensure the entries are produced from ESSENTIALS_NODES so values are typed as EssentialsCategory. For ESSENTIALS_CATEGORY_CANONICAL, change ReadonlyMap<string, string> to ReadonlyMap<string, EssentialsCategory> and ensure the mapping expression yields EssentialsCategory values (e.g., map c => [c.toLowerCase(), c as EssentialsCategory] or otherwise maintain the union type). Update only the type annotations/casts; do not change runtime logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/constants/essentialsNodes.ts`:
- Around line 81-95: The derived maps currently widen their value types to
string; change their declarations and/or casts to preserve the
EssentialsCategory union. For ESSENTIALS_CATEGORY_MAP, replace Record<string,
string> with Record<string, EssentialsCategory> (or assert the result:
Object.fromEntries(...) as Record<string, EssentialsCategory>) and ensure the
entries are produced from ESSENTIALS_NODES so values are typed as
EssentialsCategory. For ESSENTIALS_CATEGORY_CANONICAL, change
ReadonlyMap<string, string> to ReadonlyMap<string, EssentialsCategory> and
ensure the mapping expression yields EssentialsCategory values (e.g., map c =>
[c.toLowerCase(), c as EssentialsCategory] or otherwise maintain the union
type). Update only the type annotations/casts; do not change runtime logic.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
browser_tests/tests/nodeBadge.spec.ts-snapshots/node-badge-Hide-built-in-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/nodeBadge.spec.ts-snapshots/node-badge-Show-all-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (3)
src/constants/essentialsNodes.test.tssrc/constants/essentialsNodes.tssrc/constants/toolkitNodes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/constants/essentialsNodes.test.ts
christian-byrne
force-pushed
the
toolkit/wire-essentials-categorization
branch
from
13e40d0 to
2112beb
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/types/nodeSource.ts (1)
41-45: Normalize whitespace before canonical lookup.
toLowerCase()alone leaves leading/trailing spaces, so values like'Image Generation 'remain non-canonical and can propagate downstream.♻️ Proposed fix
export function getEssentialsCategory( name?: string, essentials_category?: string ): string | undefined { - const canonical = essentials_category - ? (ESSENTIALS_CATEGORY_CANONICAL.get(essentials_category.toLowerCase()) ?? - essentials_category.toLowerCase()) + const normalizedCategory = essentials_category?.trim().toLowerCase() + const canonical = normalizedCategory + ? (ESSENTIALS_CATEGORY_CANONICAL.get(normalizedCategory) ?? + normalizedCategory) : undefined return canonical ?? (name ? ESSENTIALS_CATEGORY_MAP[name] : undefined) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/nodeSource.ts` around lines 41 - 45, The canonical lookup currently uses essentials_category.toLowerCase() which preserves leading/trailing whitespace; update the logic in the canonical computation (the canonical variable) to normalize whitespace by calling trim() before lowercasing (e.g., use essentials_category.trim().toLowerCase()) and also normalize the fallback key when consulting ESSENTIALS_CATEGORY_MAP (normalize name with trim()/toLowerCase or appropriate casing) so ESSENTIALS_CATEGORY_CANONICAL and ESSENTIALS_CATEGORY_MAP lookups succeed for values like "Image Generation ".src/stores/subgraphStore.test.ts (1)
518-585: Add a conflict-precedence case foressentials_category.Current tests cover direct value and fallback, but not which source wins when both are present and disagree.
🧪 Optional test to lock precedence behavior
describe('essentials_category passthrough', () => { + it('should prefer GlobalSubgraphData essentials_category over definition fallback', async () => { + const graphWithEssentials = { + ...mockGraph, + definitions: { + subgraphs: [ + { + ...mockGraph.definitions?.subgraphs?.[0], + essentials_category: 'Image Tools' + } + ] + } + } + await mockFetch( + {}, + { + bp_precedence: { + name: 'Precedence Blueprint', + info: { node_pack: 'test_pack' }, + data: JSON.stringify(graphWithEssentials), + essentials_category: 'Video Generation' + } + } + ) + const nodeDef = useNodeDefStore().nodeDefs.find( + (d) => d.name === 'SubgraphBlueprint.bp_precedence' + ) + expect(nodeDef?.essentials_category).toBe('video generation') + }) + it('should pass essentials_category from GlobalSubgraphData to node def', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/subgraphStore.test.ts` around lines 518 - 585, Add a test case in the same "essentials_category passthrough" suite that creates a conflict (both GlobalSubgraphData/top-level blueprint object and the nested subgraph definition include differing essentials_category values) using mockFetch and the same pattern as the other tests, then find the nodeDef via useNodeDefStore().nodeDefs.find for the matching 'SubgraphBlueprint.bp_*' name and assert that the nodeDef.essentials_category equals the canonicalized value coming from the top-level/global blueprint (i.e., verify the global/top-level field takes precedence over the nested subgraph definition).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/stores/subgraphStore.test.ts`:
- Around line 518-585: Add a test case in the same "essentials_category
passthrough" suite that creates a conflict (both GlobalSubgraphData/top-level
blueprint object and the nested subgraph definition include differing
essentials_category values) using mockFetch and the same pattern as the other
tests, then find the nodeDef via useNodeDefStore().nodeDefs.find for the
matching 'SubgraphBlueprint.bp_*' name and assert that the
nodeDef.essentials_category equals the canonicalized value coming from the
top-level/global blueprint (i.e., verify the global/top-level field takes
precedence over the nested subgraph definition).
In `@src/types/nodeSource.ts`:
- Around line 41-45: The canonical lookup currently uses
essentials_category.toLowerCase() which preserves leading/trailing whitespace;
update the logic in the canonical computation (the canonical variable) to
normalize whitespace by calling trim() before lowercasing (e.g., use
essentials_category.trim().toLowerCase()) and also normalize the fallback key
when consulting ESSENTIALS_CATEGORY_MAP (normalize name with trim()/toLowerCase
or appropriate casing) so ESSENTIALS_CATEGORY_CANONICAL and
ESSENTIALS_CATEGORY_MAP lookups succeed for values like "Image Generation ".
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/constants/essentialsNodes.test.tssrc/constants/essentialsNodes.tssrc/stores/subgraphStore.test.tssrc/types/nodeSource.test.tssrc/types/nodeSource.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/constants/essentialsNodes.ts
christian-byrne
force-pushed
the
toolkit/wire-essentials-categorization
branch
from
2112beb to
d2de2fc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/types/nodeSource.test.ts`:
- Around line 113-120: The test name says it should normalize title-cased
backend categories but only asserts classification; update the test using
getNodeSource to verify normalization explicitly: call getNodeSource with 'Image
Generation' and assert that the returned category is normalized (e.g., equals
'image generation') or that the result is identical to calling getNodeSource
with the already-lowercased 'image generation'; keep the existing
NodeSourceType.Essentials assertion but add the normalization check so the test
fails if getNodeSource stops lowercasing categories.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/constants/essentialsNodes.test.tssrc/constants/essentialsNodes.tssrc/stores/subgraphStore.test.tssrc/types/nodeSource.test.tssrc/types/nodeSource.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/constants/essentialsNodes.ts
- src/constants/essentialsNodes.test.ts
…tive backend matching Backend sends title-cased categories (e.g. 'Image Generation') while frontend uses lowercase (e.g. 'image generation'). Add canonical lookup map to resolve backend values to the exact form in ESSENTIALS_CATEGORIES, preserving special cases like '3D'. Addresses review feedback from benceruleanlu. Amp-Thread-ID: https://ampcode.com/threads/T-019c93b8-dae2-77ba-92c5-c7a71a424a3f
christian-byrne
force-pushed
the
toolkit/wire-essentials-categorization
branch
from
d2de2fc to
fbebac4
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/types/nodeSource.test.ts (1)
113-123: Past review comment addressed – test now asserts both classification and normalization.The added
expect(getEssentialsCategory('SomeNode', 'Image Generation')).toBe('image generation')closes the gap flagged in the previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/nodeSource.test.ts` around lines 113 - 123, The test now verifies both classification and normalization by calling getNodeSource('nodes.some_module','Image Generation','SomeNode') and asserting result.type === NodeSourceType.Essentials, and also checks getEssentialsCategory('SomeNode','Image Generation') === 'image generation'; no code changes required—keep the assertions as written to ensure both behavior and normalization are covered (verify getNodeSource and getEssentialsCategory remain unchanged and return the expected values).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/types/nodeSource.test.ts`:
- Around line 113-123: The test now verifies both classification and
normalization by calling getNodeSource('nodes.some_module','Image
Generation','SomeNode') and asserting result.type === NodeSourceType.Essentials,
and also checks getEssentialsCategory('SomeNode','Image Generation') === 'image
generation'; no code changes required—keep the assertions as written to ensure
both behavior and normalization are covered (verify getNodeSource and
getEssentialsCategory remain unchanged and return the expected values).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/constants/essentialsNodes.test.tssrc/constants/essentialsNodes.tssrc/stores/subgraphStore.test.tssrc/types/nodeSource.test.tssrc/types/nodeSource.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/types/nodeSource.ts
- src/constants/essentialsNodes.test.ts
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Wire
essentials_categorythrough from backend to the Essentials tab UI. Creates a single source of truth for node categorization and ordering.Changes
New file —
src/constants/essentialsNodes.ts:ESSENTIALS_NODES(ordered nodes per category),ESSENTIALS_CATEGORIES(folder display order),ESSENTIALS_CATEGORY_MAP(flat lookup),TOOLKIT_NOVEL_NODE_NAMES(telemetry),TOOLKIT_BLUEPRINT_MODULESRefactored files:
src/types/nodeSource.ts: Removed inlineESSENTIALS_CATEGORY_MOCK, importsESSENTIALS_CATEGORY_MAPfrom centralized constantssrc/services/nodeOrganizationService.ts: Removed inlineNODE_ORDER_BY_FOLDER, importsESSENTIALS_NODESandESSENTIALS_CATEGORIESsrc/constants/toolkitNodes.ts: Re-exports fromessentialsNodes.tsinstead of maintaining a separate listSubgraph passthrough:
src/stores/subgraphStore.ts: Passesessentials_categoryfromGlobalSubgraphDataand extracts it fromdefinitions.subgraphs[0]as fallbacksrc/platform/workflow/validation/schemas/workflowSchema.ts: Addedessentials_categorytoSubgraphDefinitionBaseandzSubgraphDefinitionTests:
src/constants/essentialsNodes.test.ts: 6 tests validating no duplicates, complete coverage, basics exclusionsrc/stores/subgraphStore.test.ts: 2 tests for essentials_category passthroughAll 43 relevant tests pass. Typecheck, lint, format clean.
Depends on: Comfy-Org/ComfyUI#12573
Fixes COM-15221
┆Issue is synchronized with this Notion page by Unito