chore: salvage remaining UI polish from #3344#3353
Conversation
Ports the parts of #3344 that were not absorbed by the merged Fuselage migration (#3349): - Replace deprecated bare color tokens with explicit font-* tokens (font-default, font-hint, font-annotation) in ServerInfoContent, DocumentViewer, MarkdownContent, PdfContent, and TopBar - Replace the fake-link action button in CertificatesManager with a proper Fuselage Button - Use the Inter-first font stack in GlobalStyles instead of system-ui - Commit the design context files (DESIGN.md, PRODUCT.md, .impeccable/design.json) that #3344 introduced
WalkthroughThis PR establishes a comprehensive design system for Rocket.Chat Desktop and applies new design tokens throughout the UI. It introduces design documentation, schema, and token definitions, then updates existing components to use the new token vocabulary while refactoring ActionButton for consistency. ChangesDesign System Implementation and Token Migration
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
DESIGN.md (1)
250-256: ⚡ Quick winNormalize the shadow token name.
Section 4 defines
elevation-2x/elevation-2y, but the dropdown spec later refers toelevation-2. Please verify the canonical Fuselage token name and make the doc consistent before it becomes an implementation contract break.Also applies to: 313-319
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DESIGN.md` around lines 250 - 256, The doc uses both elevation-2x / elevation-2y and elevation-2 inconsistently; pick the canonical Fuselage shadow token name (confirm whether Fuselage exposes elevation-2 or separate elevation-2x/elevation-2y) and update all references in this file (the Shadow Vocabulary section and the dropdown/dialog specs at the later block referenced around lines 313-319) to the chosen token name so the design text matches Fuselage's actual token API (e.g., replace elevation-2x/elevation-2y with elevation-2, or vice versa, everywhere)..impeccable/design.json (1)
214-215: ⚡ Quick winRemove the literal shadows from the examples.
The spec says shell-owned CSS must not author
box-shadow, but these dialog and dropdown snippets still hardcode it. That makes the contract self-contradictory and easy to copy into implementation.Also applies to: 230-231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.impeccable/design.json around lines 214 - 215, The dialog/dropdown snippets include hardcoded box-shadow (e.g., in the .ds-dialog CSS) which violates the shell-owned rule; remove the literal box-shadow declarations from the snippet(s) and either omit the property or replace it with a shell-controlled token (e.g., use a CSS variable like var(--rcx-shadow) instead of the literal shadow) so .ds-dialog (and the corresponding dropdown snippet) no longer contain hardcoded box-shadow values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.impeccable/design.json:
- Around line 214-215: The dialog/dropdown snippets include hardcoded box-shadow
(e.g., in the .ds-dialog CSS) which violates the shell-owned rule; remove the
literal box-shadow declarations from the snippet(s) and either omit the property
or replace it with a shell-controlled token (e.g., use a CSS variable like
var(--rcx-shadow) instead of the literal shadow) so .ds-dialog (and the
corresponding dropdown snippet) no longer contain hardcoded box-shadow values.
In `@DESIGN.md`:
- Around line 250-256: The doc uses both elevation-2x / elevation-2y and
elevation-2 inconsistently; pick the canonical Fuselage shadow token name
(confirm whether Fuselage exposes elevation-2 or separate
elevation-2x/elevation-2y) and update all references in this file (the Shadow
Vocabulary section and the dropdown/dialog specs at the later block referenced
around lines 313-319) to the chosen token name so the design text matches
Fuselage's actual token API (e.g., replace elevation-2x/elevation-2y with
elevation-2, or vice versa, everywhere).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf280ccd-7b84-4482-b05f-185ca5f9c276
📒 Files selected for processing (10)
.impeccable/design.jsonDESIGN.mdPRODUCT.mdsrc/ui/components/CertificatesManager/ActionButton.tsxsrc/ui/components/ServerInfoContent.tsxsrc/ui/components/ServersView/DocumentViewer.tsxsrc/ui/components/ServersView/MarkdownContent.tsxsrc/ui/components/ServersView/PdfContent.tsxsrc/ui/components/Shell/styles.tsxsrc/ui/components/TopBar/index.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: build (windows-latest, windows)
- GitHub Check: build (ubuntu-latest, linux)
- GitHub Check: check (macos-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/ui/components/ServersView/DocumentViewer.tsxsrc/ui/components/CertificatesManager/ActionButton.tsxsrc/ui/components/ServersView/PdfContent.tsxsrc/ui/components/Shell/styles.tsxsrc/ui/components/TopBar/index.tsxsrc/ui/components/ServersView/MarkdownContent.tsxsrc/ui/components/ServerInfoContent.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/ui/components/ServersView/DocumentViewer.tsxsrc/ui/components/CertificatesManager/ActionButton.tsxsrc/ui/components/ServersView/PdfContent.tsxsrc/ui/components/Shell/styles.tsxsrc/ui/components/TopBar/index.tsxsrc/ui/components/ServersView/MarkdownContent.tsxsrc/ui/components/ServerInfoContent.tsx
🪛 ast-grep (0.43.0)
src/ui/components/ServersView/MarkdownContent.tsx
[warning] 237-237: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(react-unsafe-html-injection)
🪛 LanguageTool
DESIGN.md
[grammar] ~173-~173: Ensure spelling is correct
Context: ...ients, no decorative iconography in the titlebar, no busy toolbar). It is not generic Sa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~173-~173: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...aphy in the titlebar, no busy toolbar). It is not generic SaaS marketing (no hero ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~344-~344: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rast before merging a color change. - Do use sentence case in source strings; ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~349-~349: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...y, no animated reactions in chrome. - Don't revive old Skype or legacy Teams c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~350-~350: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lebars, no busy multi-row toolbars. - Don't use generic SaaS marketing pattern...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~351-~351: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ds, no gradient accents on numbers. - Don't pursue pure brand minimalism (Line...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
PRODUCT.md
[grammar] ~15-~15: Ensure spelling is correct
Context: ...l around one or more Rocket.Chat server webviews. Its job is to make managing multiple w...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~32-~32: Ensure spelling is correct
Context: ...esktop chrome must not compete with the webview. Sidebar, titlebar, and dialogs exist t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (8)
src/ui/components/Shell/styles.tsx (1)
37-47: LGTM!src/ui/components/CertificatesManager/ActionButton.tsx (1)
1-2: LGTM!Also applies to: 4-4, 7-7
PRODUCT.md (1)
1-53: LGTM!src/ui/components/ServerInfoContent.tsx (1)
127-152: LGTM!Also applies to: 181-185, 260-305, 328-387
src/ui/components/ServersView/DocumentViewer.tsx (1)
32-39: LGTM!src/ui/components/ServersView/MarkdownContent.tsx (1)
200-208: LGTM!Also applies to: 231-238
src/ui/components/ServersView/PdfContent.tsx (1)
74-82: LGTM!src/ui/components/TopBar/index.tsx (1)
21-30: LGTM!
Linux installer download |
macOS installer download |
1 participant
Summary
#3349 absorbed most of #3344's UI polish, but a set of changes never made it to
master. This PR ports the remainder, verified file-by-file against the merged state:font-*color tokens —color='hint' | 'default' | 'annotation'→'font-hint' | 'font-default' | 'font-annotation'inServerInfoContent,DocumentViewer,MarkdownContent,PdfContent, andTopBar, matching the token names in Fuselage'sTheme.d.ts.<a href='#'>fake-link wrapped in aBoxwith a proper Fuselage<Button small secondary>, fixing semantics and keyboard accessibility.GlobalStylesnow uses the Inter-first stack instead of baresystem-ui.DESIGN.md,PRODUCT.md, and.impeccable/design.jsonintroduced by chore: Polish Settings, Certificates, and Downloads views against Fuselage #3344 (current versions, updated after the Fuselage 0.78 migration).Deliberately not ported from #3344: the
Section/CertificateSectioncomponent restructure and its i18n keys — #3349 shipped a different design for those views, and porting them would revert it.Verification
yarn lintandnpx tsc --noEmitpass on this branch.Supersedes the remaining scope of #3344.
Summary by CodeRabbit
Documentation
Style
Refactor