fix(studio): unify entity icons behind a single canonical registry [ASTD-447] - #1531
Open
aahunt-nv wants to merge 5 commits into
Open
fix(studio): unify entity icons behind a single canonical registry [ASTD-447]#1531aahunt-nv wants to merge 5 commits into
aahunt-nv wants to merge 5 commits into
Conversation
Signed-off-by: Aaron Hunt <aahunt@nvidia.com>
Nav icons and empty-state icons were two independent sources of truth with nothing linking them, so they drifted: 9 of 19 entities showed a different glyph in the sidebar than in their own empty state, and three fell through to a FolderOpen default. ShieldCheck and Radar each covered two unrelated entities, so neither glyph meant anything specific. Add ENTITY_ICONS as the one canonical entity -> glyph map, and remove the icon field from EmptyStateDescriptor so a descriptor can no longer carry a conflicting glyph. Route groups and the workspace side nav now render from the same map, making the mismatch unrepresentable rather than merely fixed. Where an entity had a nav glyph, that glyph wins -- it is the persistent, most-seen representation. Sub-entities inherit their parent's family, so guardrail configs keep the keyhole shield and their tests take the check. Refs: ASTD-447 Signed-off-by: Aaron Hunt <aahunt@nvidia.com>
Contributor
|
Guardrails and its tests both take ShieldCheck -- a config and its test cases are one thing to the user, so the tests inherit the config glyph rather than claiming a second shield. Jobs keeps ListChecks; it is a persistent nav item, where guardrailChecks appears only in one empty state nested two levels into a config. Datasets is becoming an entity in its own right, so it gets a canonical entry now rather than staying the one nav glyph outside the map. That makes ENTITY_ICONS a superset of the empty-state registry, so split EmptyStateEntityKey out of EntityKey. EntityEmptyState accepts only the narrower type, which keeps 'every empty state resolves an icon' a compile- time guarantee while letting an entity have an icon before it has copy. Refs: ASTD-447 Signed-off-by: Aaron Hunt <aahunt@nvidia.com>
Monitor runs are agent telemetry -- the list is populated from the nemo-agent-telemetry fileset -- so they read as the same data spans are drawn from, and take Logs rather than the Bot glyph that stands for the agent itself. Recorded as an intentional alias against telemetrySpans, so the no-reuse guard still holds and the shared glyph is a stated decision rather than a collision that slipped through. Refs: ASTD-447 Signed-off-by: Aaron Hunt <aahunt@nvidia.com>
Icons render at 16px via the .lucide base rule, where lucide's native stroke reads heavier than intended against 14px text. Pin the stroke to 1. This restores the override removed in #1369, which had dropped it to fall back to lucide's upstream default of 2. Reinstating it deliberately: the lighter weight is the wanted look for Studio's 16px icons. Note that the adjacent vector-effect: non-scaling-stroke rule means the stroke is drawn in the scaled coordinate space, so the on-screen result is lighter than the authored value. Refs: ASTD-447 Signed-off-by: Aaron Hunt <aahunt@nvidia.com>
aahunt-nv
marked this pull request as ready for review
August 25, 2026 22:32
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (20)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe PR adds a canonical entity icon registry, updates empty-state rendering to use it, and replaces direct Lucide icons across Studio navigation. It also adds registry tests and sets the default Lucide stroke width to 1. ChangesCanonical entity icon adoption
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change unifies entity icons across navigation and empty states while preserving compile-time coverage and existing behavior outside icon presentation. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 19 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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
1 participant
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.
Resolves: ASTD-447
Summary
Implements Surface 3 from the audit — nav icon vs. empty-state icon divergence. (The two Agent sub-pages in Surface 1 are being removed separately.)
The audit found 6 match / 9 mismatch / 4 no-nav-icon across 19 entities. The cause is structural: there were two independent icon sources with nothing linking them.
web/packages/studio/src/routes/groups/*Routes.tsx— each route group imported its ownlucide-reactglyph forslotIcon.web/packages/common/src/components/EntityEmptyState/registry.ts— each descriptor carried its owniconfield.Hand-fixing the 9 would drift again on the next entity added, so this makes the mismatch unrepresentable instead:
web/packages/common/src/constants/entityIcons.tsholdsENTITY_ICONS— the one canonical entity → glyph map.iconis removed fromEmptyStateDescriptor.EntityEmptyStateresolvesENTITY_ICONS[entity]. A descriptor can no longer specify a conflicting glyph.WorkspaceSideNavrender from the same map. NoslotIconin Studio imports its glyph locally anymore.ENTITY_ICONSis a superset of the empty-state registry, since an entity can have an icon before it has empty-state copy.EmptyStateEntityKeynarrowsEntityKey, andEntityEmptyStateaccepts only the narrower type — so "every empty state resolves an icon" stays a compile-time guarantee.Icon decisions
Where an entity had a nav glyph, that glyph wins — it's the persistent, most-frequently-seen representation.
agentsBotHatGlassesagentMonitorRunsLogsbaseModelsBoxesFolderOpenfallthroughcustomModelsMetronomeBrainCircuitvirtualModelsWaypointsRadar, which freesRadarfor providersdatasetsDatabasefilesetsFileStackDatabaseanonymizerJobsUserPenVenetianMaskdataDesignerJobsFormLightbulb, which collided with InsightssafeSynthesizerJobsDatabaseCheckShieldCheckguardrails,guardrailChecksShieldCheckevaluationResults,agentEvaluations,evaluationSessionsChartBarFlaskConicalevalComparisonGitCompareChartNetworktelemetryTraces,insightTracesListTreeAnchortelemetrySpansLogsAnchorUnchanged:
deployments(Rocket),inferenceProviders(Radar),experiments/insightExperiments(FlaskConical),optimizerInsights(Lightbulb),jobs(ListChecks),filesetFiles(FolderOpen),secrets(LockKeyhole),members(UsersRound).Four calls worth surfacing:
ShieldCheck. A config and its test cases are one thing to the user, so the tests inherit the config glyph rather than claiming a second shield. This retiresShieldKeyholeand leaves the shield family meaning guardrails only — Safe Synthesizer moves toDatabaseCheck, its nav glyph.jobskeepsListChecks. It is a persistent sidebar item;guardrailChecksrenders in exactly one empty state nested two levels into a config, so the nav item has the stronger claim.agentMonitorRunssharesLogswithtelemetrySpans. The Monitor list is populated from thenemo-agent-telemetryfileset, so it is the same underlying data —Botis reserved for the agent itself.datasetsgets a canonical entry even though it is a nav grouping today, because it is becoming an entity in its own right. It would otherwise be the one nav glyph left outside the map.Testing
entityIcons.test.ts: every empty-state entity resolves an icon, aliases point at their parent, and no glyph is reused across unrelated entities — the regression guard for theShieldCheck/Radarcollisions. Intentional aliases are declared in one table with a reason each, so adding a collision means editing that table on purpose.Verified non-vacuous: pointing
jobsatShieldCheckfails withexpected [['guardrails','jobs']] to deeply equal [].pnpm --filter @nemo/common test— 1513 passed.pnpm --filter nemo-studio-ui test— 2960 passed.AssistantChatRoute/api.test.tsfails withlocalStorage.clear is not a function; verified the identical 18 failures onmain, so pre-existing and unrelated.typecheckclean on both packages (catches any misseddescriptor.icon),lintclean (catches orphanedlucide-reactimports).Icon stroke weight
.lucideinweb/packages/studio/src/index.cssalready sizes every icon to 16px. This pins itsstroke-widthto1for a lighter weight against 14px text.This reinstates the override that #1369 removed a week ago (which had dropped it to fall back to lucide's upstream default of
2). Calling that out so the flip-flop ingit logis intentional and legible rather than looking like an accidental revert.Worth knowing for anyone tuning this later: the adjacent
vector-effect: non-scaling-strokerule means the stroke is drawn in the scaled coordinate space, so the authored value is not the on-screen pixel value — lucide's geometry is a 24-unit viewBox scaled to 16px. An authored1therefore renders lighter than 1px. I was not able to measure the exact rendered width in a browser here, so if the weight looks off in review, that scaling is the thing to adjust against.Summary by CodeRabbit
UI Improvements
Bug Fixes