style: update ui and design of system notification components (what's new, new release notification, help center) #6300
Conversation
christian-byrne
requested review from
a team,
KarryCharon,
Yorha4D and
shinshin86
as code owners
dosubot
bot
added
the
size:L
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 11/07/2025, 06:17:03 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 11/07/2025, 06:31:48 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.25 MB (baseline 3.25 MB) • 🔴 +138 BMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 795 kB (baseline 793 kB) • 🔴 +1.89 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 8.18 kB (baseline 8.18 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 293 kB (baseline 293 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 12.6 kB (baseline 12.6 kB) • ⚪ 0 BReusable component library chunks
Status: 1 added / 1 removed Data & Services — 10.4 kB (baseline 10.4 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 1 added / 1 removed Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 2.55 MB (baseline 2.55 MB) • ⚪ 0 BBundles that do not match a named category
|
christian-byrne
added
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| style="margin-left: auto" | ||
| /> | ||
| </button> | ||
| </nav> |
There was a problem hiding this comment.
[security] high Priority
Issue: Potential XSS vulnerability with v-html usage
Context: Line 45 uses v-html with formattedContent which could allow arbitrary HTML injection if content is not properly sanitized
Suggestion: Use DOMPurify for sanitization before rendering or use safe markdown rendering approach. Check if renderMarkdownToHtml already includes sanitization
| width: 12px; | ||
| height: 2px; | ||
| background: white; | ||
| background: var(--text-primary); |
There was a problem hiding this comment.
[quality] low Priority
Issue: Inconsistent semantic token usage for text colors
Context: Line uses var(--text-primary) for both background and color properties which seems incorrect
Suggestion: Verify that using text tokens for background colors is intentional and semantically correct
| icon: 'icon-[lucide--clipboard-pen]', | ||
| label: t('helpCenter.feedback'), | ||
| action: () => { | ||
| // TODO: Implement feedback dialog action |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Unimplemented functionality with TODO comment
Context: The feedback button action contains a TODO comment indicating incomplete implementation
Suggestion: Either implement the feedback dialog action or remove the feedback button if it's not ready for this release. Avoid shipping incomplete features with TODO markers
| <i | ||
| v-if="menuItem.showExternalIcon" | ||
| class="icon-[lucide--external-link] text-text-primary" | ||
| style="width: 16px; height: 16px; margin-left: auto" |
There was a problem hiding this comment.
[quality] low Priority
Issue: Inline styles instead of CSS classes
Context: Lines 37 and 42 use inline styles for icon sizing and positioning that could be moved to CSS classes
Suggestion: Create CSS classes like .external-icon and .chevron-icon to replace inline styles for better maintainability and consistency
There was a problem hiding this comment.
Or just use tailwind utility classes.
| <span class="menu-label">{{ menuItem.label }}</span> | ||
| <i | ||
| v-if="menuItem.showExternalIcon" | ||
| class="icon-[lucide--external-link] text-text-primary" |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Redundant CSS class naming - text-text-primary
Context: Line uses text-text-primary which appears to have redundant text- prefix
Suggestion: Use just text-primary or verify if this is the correct class name. The current naming suggests it should be just text-primary
|
|
||
| // Verify help center menu appears | ||
| const helpMenu = comfyPage.page.locator('.help-center-menu') | ||
| await expect(helpMenu).toBeVisible() |
There was a problem hiding this comment.
[architecture] low Priority
Issue: Test cases do not verify accessibility compliance
Context: New UI components and updated menu structure should include accessibility testing
Suggestion: Add test cases to verify ARIA attributes, keyboard navigation, and screen reader compatibility for the updated help center menu and notification components
There was a problem hiding this comment.
no, this is not necessary.
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: style: update ui and design of system notification components (what's new, new release notification, help center) (#6300)
Impact: 190 additions, 159 deletions across 6 files
Issue Distribution
- Critical: 0
- High: 0
- Medium: 4
- Low: 2
Category Breakdown
- Architecture: 1 issues
- Security: 0 issues
- Performance: 0 issues
- Code Quality: 5 issues
Key Findings
Architecture & Design
The PR correctly implements semantic design tokens following the established pattern with --interface-menu-surface and --interface-menu-stroke. The token naming follows existing conventions and properly supports light/dark theme switching. However, new design tokens lack documentation which could make them difficult for other developers to understand and use correctly.
Security Considerations
No new security vulnerabilities introduced. The changes are primarily cosmetic, replacing hardcoded colors with semantic tokens. The existing v-html usage in WhatsNewPopup was not modified and appears to use proper sanitization through renderMarkdownToHtml.
Performance Impact
No significant performance regressions identified. The changes replace static color values with CSS custom properties which have negligible performance impact. The computed property structure in HelpCenterMenuContent appears well-organized without circular dependencies.
Integration Points
The changes maintain backward compatibility and follow the existing component architecture. The help center UI improvements (new "Give Feedback" button, icon updates) align with the design system evolution. Tests appropriately cover the functionality changes.
Positive Observations
- Consistent use of semantic design tokens throughout all components
- Proper maintenance of existing accessibility attributes (role, aria-label)
- Comprehensive test coverage for the release notification functionality
- Clean separation of concerns between styling and logic changes
- Follows Vue 3 Composition API best practices
Issues to Address
Medium Priority
- Missing documentation for new CSS design tokens - add inline comments explaining purpose
- Unimplemented functionality - feedback button contains TODO comment, should be implemented or removed
- CSS class naming - text-text-primary appears redundant, should be text-primary
- Semantic token usage - verify using text tokens for background colors is semantically correct
Low Priority
- Inline styles - replace with CSS classes for better maintainability
- Accessibility testing - add test cases for ARIA compliance and keyboard navigation
References
- Repository Architecture Guide
- ComfyUI Design System Guidelines
- CLAUDE.md project guidelines in repository
Next Steps
- Address medium priority issues before merge, especially the unimplemented feedback functionality
- Consider adding documentation for design tokens to improve maintainability
- Review CSS class naming consistency across components
- Consider adding accessibility test coverage for enhanced UI components
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
- Add proper semantic design tokens (corner-radius, blue surface) - Convert hardcoded colors to theme-aware tokens in toast and popup - Implement Figma-compliant styling with image support - Add comprehensive component tests for behavioral validation - Update translation strings for improved UI copy
- Remove temporary mock data from WhatsNewPopup component - Fix i18n mocking issues in both test suites - Fix TypeScript errors with vi.mocked calls - Update failing tests to use direct method calls instead of DOM events - Remove duplicate/outdated test file - All 22 tests now passing
christian-byrne
force-pushed
the
style/whatsnew-light-mode
branch
from
d9eef91 to
f62a5e4
Compare
Summary
Migrated help center and release notification components from hardcoded colors to semantic design tokens for automatic light/dark theme support.
Changes
--interface-menu-surfaceand--interface-menu-stroketokens to style.css for consistent menu theming┆Issue is synchronized with this Notion page by Unito