feat: theme + ux overhaul #6520
Conversation
helloanoop
requested review from
bijin-bruno,
lohit-bruno and
naman-bruno
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMajor theming system overhaul introducing variant-based theme selection (light/dark with multiple palette options), refactoring component styling to use centralized theme tokens, replacing native buttons with a shared Button component, restructuring GrpcBody and WsBody message UIs with dedicated toolbars, and removing legacy components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive theme system overhaul for the Bruno application, adding support for multiple custom themes with a schema-based validation system. The changes include 11 new theme variants (both light and dark), a JSON schema for theme validation, enhanced theme provider logic, and UI component updates to use the new theme tokens.
Key Changes
- Theme System Architecture: Added JSON schema validation for themes and a registry-based theme management system
- Multiple Theme Variants: Introduced 11 theme variants including Catppuccin (4 variants), VS Code (2 variants), Nord, Monochrome (2 variants), Pastel (2 variants), and the default themes
- UI Component Updates: Updated Button and ResponsiveTabs components to use new theme tokens and improved button styling consistency
Reviewed changes
Copilot reviewed 121 out of 123 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/bruno-app/src/ui/ResponsiveTabs/StyledWrapper.js |
Updated tab colors to use new theme token theme.colors.text.subtext0 |
packages/bruno-app/src/ui/Button/StyledWrapper.js |
Adjusted button font sizes and added border property support for button variants |
packages/bruno-app/src/themes/schema/oss.js |
Added comprehensive JSON schema defining the structure and required properties for themes |
packages/bruno-app/src/themes/schema/index.js |
Exports the OSS theme schema |
packages/bruno-app/src/themes/light/*.js |
Added 5 new light theme variants (light, light-monochrome, light-pastel, catppuccin-latte, vscode) |
packages/bruno-app/src/themes/dark/*.js |
Added 6 new dark theme variants (dark, dark-monochrome, dark-pastel, catppuccin themes, nord, vscode-dark) |
packages/bruno-app/src/themes/index.js |
Updated theme exports with registry metadata and helper functions for theme filtering |
packages/bruno-app/src/styles/globals.css |
Removed unused CSS variable --color-text-link |
packages/bruno-app/src/styles/app.scss |
Removed file content (buttons import removed) |
packages/bruno-app/src/providers/Theme/index.js |
Enhanced theme provider with variant support, schema validation, and toast notifications |
packages/bruno-app/src/providers/ReduxStore/slices/app.js |
Added preferences tab state management for navigation |
packages/bruno-app/src/components/WorkspaceHome/WorkspaceOverview/index.js |
Replaced custom buttons with Button component for consistency |
packages/bruno-app/src/components/WorkspaceHome/WorkspaceOverview/CollectionsList/index.js |
Updated dropdown item class from dropdown-item-danger to delete-item |
packages/bruno-app/src/components/ToolHint/StyledWrapper.js |
Removed unused styled component file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,17 +1,18 @@ | |||
| import styled from 'styled-components'; | |||
| import { darken, rgba } from 'polished'; | |||
There was a problem hiding this comment.
Unused import rgba.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/bruno-app/src/ui/ResponsiveTabs/StyledWrapper.js (1)
49-49: Inconsistent theming pattern detected.Line 49 uses
props.theme.textwhile the updated lines (15, 27) now use the more specificprops.theme.colors.text.subtext0pattern. This inconsistency suggests line 49 may have been overlooked during the theme migration.🔎 Proposed fix to align with new theming pattern
.content-indicator { - color: ${(props) => props.theme.text}; + color: ${(props) => props.theme.colors.text.subtext0}; }packages/bruno-app/src/components/ShareCollection/index.js (1)
34-35: Semantic type mismatch inhasproperty.The
hasproperty name implies a boolean value, but line 35 can return either a number (the length) orfalse. While this works functionally in the boolean context on line 115, it's semantically inconsistent.🔎 Suggested fix
- return { - has: collection?.items?.filter(checkItem).length || false, - types: [...types] - }; + return { + has: Boolean(collection?.items?.filter(checkItem).length), + types: [...types] + };packages/bruno-app/src/components/ManageWorkspace/StyledWrapper.js (1)
86-86: Inconsistent theme token migration in the same file.Lines 86, 159, and 169 use
theme.colors.text.muted/danger, while line 110 was migrated totheme.text.muted. Standardize all color tokens within this file to use the same path structure.Suggested changes:
- Line 86: Change
theme.colors.text.mutedtotheme.text.muted- Line 159: Change
theme.colors.text.dangertotheme.text.danger- Line 169: Change
theme.colors.text.mutedtotheme.text.mutedpackages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js (1)
8-10: Complete the theme migration for hover states.Lines 5 and 21 correctly use theme props, but the hover states on Lines 9 and 24 still reference CSS variables. For consistency with the theming overhaul, migrate these to theme-based values as well.
🔎 Proposed fix to complete theme migration
button { color: ${(props) => props.theme.colors.text.subtext0}; cursor: pointer; &:hover { - color: var(--color-tab-active); + color: ${(props) => props.theme.tabs.active.color}; } &:disabled { cursor: not-allowed; opacity: 0.5; } } .cursor-pointer { display: flex; align-items: center; color: ${(props) => props.theme.colors.text.subtext0}; &:hover { - color: var(--color-tab-active); + color: ${(props) => props.theme.tabs.active.color}; } }Based on learnings, styled components should use the theme prop to manage CSS colors rather than CSS variables.
Also applies to: 23-25
packages/bruno-app/src/themes/light/light-monochrome.js (1)
260-265: Missing#prefix on color value.Line 261 has
color: '212529'which should becolor: '#212529'. This will cause the close button text color to not render correctly.🔎 Proposed fix
close: { - color: '212529', + color: '#212529', bg: 'white', border: 'white', hoverBorder: '' },packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/StyledWrapper.js (1)
47-49: Replacecolors.text.bluewith an existing theme property. The propertycolors.text.bluedoesn't exist in any theme file. All themes define only:white,green,danger,warning,muted,purple,yellow,subtext0,subtext1, andsubtext2. This same issue exists in WsResponsePane as well. Use an appropriate existing color likecolors.text.mutedorcolors.text.warninginstead.packages/bruno-app/src/providers/Theme/index.js (1)
27-33: Memory leak: event listener never removed.The
changeevent listener is added to the media query but never cleaned up. This will accumulate listeners on re-renders or if the provider remounts.🔎 Proposed fix
useEffect(() => { - window.matchMedia('(prefers-color-scheme: light)').addEventListener('change', (e) => { + const mediaQuery = window.matchMedia('(prefers-color-scheme: light)'); + const handleChange = (e) => { if (storedTheme !== 'system') return; setDisplayedTheme(e.matches ? 'light' : 'dark'); toggleHtml(); - }); + }; + mediaQuery.addEventListener('change', handleChange); + return () => mediaQuery.removeEventListener('change', handleChange); }, []);Note: With this fix, consider adding
storedThemeto the dependency array so the handler reflects the current stored theme value, or use a ref to avoid stale closure issues.
♻️ Duplicate comments (1)
packages/bruno-app/src/components/Sidebar/ImportCollectionLocation/StyledWrapper.js (1)
2-2: Remove unused import.The
rgbautility is imported but never used in this file. Onlydarkenis utilized for the hover states.🔎 Proposed fix
-import { darken, rgba } from 'polished'; +import { darken } from 'polished';
🧹 Nitpick comments (16)
packages/bruno-app/src/themes/PaletteViewer/Catppuccin.stories.jsx (1)
153-210: Refactor components to use styled components instead of inline styles.The
ColorSwatchandThemeSectioncomponents use inline styles to define colors, which conflicts with the project's coding guidelines. Per the standards, styled components with the theme prop should manage CSS colors rather than inline styles.While this is a palette viewer showcasing raw color values, maintaining consistency with the codebase's styling approach is recommended.
As per coding guidelines, styled components should be used as wrappers to define styling, with colors managed via the theme prop.
packages/bruno-app/src/themes/schema/oss.js (1)
1-4: Consider adding$schemaproperty for JSON Schema version clarity.Adding a
$schemaproperty helps document which JSON Schema draft version is being used and can improve tooling support.🔎 Proposed addition
export const ossSchema = { + $schema: 'http://json-schema.org/draft-07/schema#', type: 'object', properties: { mode: { type: 'string', description: 'Theme mode', enum: ['light', 'dark'] },packages/bruno-app/src/components/ShareCollection/index.js (1)
118-123: Minor spacing improvement for warning text.The warning text flows without spacing between "Note:" and the request types list. Adding a space would improve readability.
🔎 Suggested improvement
<span> - Note: + Note:{' '} {hasNonExportableRequestTypes.types.join(', ')} {' '} requests in this collection will not be exported </span>packages/bruno-app/src/components/RequestPane/RequestBody/RequestBodyMode/StyledWrapper.js (1)
17-18: Consider migrating hardcoded caret color to theme token.The
.caretselector still usesrgb(140, 140, 140). For consistency with the theme-based approach adopted elsewhere in this PR, consider using a theme token liketheme.colors.text.muted.packages/bruno-app/src/components/RequestPane/GrpcBody/StyledWrapper.js (1)
112-116: Hardcoded font size should use theme token.For consistency with the rest of the file, use the theme's font size tokens.
p { color: ${(props) => props.theme.colors.text.muted}; - font-size: 13px; + font-size: ${(props) => props.theme.font.size.base}; }packages/bruno-app/src/components/RequestPane/GrpcBody/index.js (1)
271-283: Array index as key may cause state issues when deleting messages.When a message is deleted, React may incorrectly preserve state for subsequent items due to index-based keys. Consider using a stable identifier.
- {messagesToShow.map((message, index) => ( + {messagesToShow.map((message, index) => { + const messageKey = message.name || `msg-${index}`; + return ( <SingleGrpcMessage - key={index} + key={messageKey} message={message} ... /> + ); + })} - ))}Alternatively, add a unique
idfield when creating messages.packages/bruno-app/src/themes/PaletteViewer/PaletteViewer.stories.jsx (2)
174-182: Hue extraction doesn't handle hex colors.The PRIMARY accent uses hex
#bd7a28, but the hue regex only matcheshsl()format. This causes PRIMARY to sort with hue 0°.const sorted = [...accents].sort((a, b) => { - const hueA = parseInt(a[1].match(/hsl\((\d+)/)?.[1] || 0); - const hueB = parseInt(b[1].match(/hsl\((\d+)/)?.[1] || 0); + const getHue = (color) => { + const hslMatch = color.match(/hsl\((\d+)/); + if (hslMatch) return parseInt(hslMatch[1]); + // For hex, approximate hue (or exclude from sorting) + return 0; + }; + const hueA = getHue(a[1]); + const hueB = getHue(b[1]); return hueA - hueB; });Consider converting PRIMARY to hsl() for consistency with other accents, or enhance the hue extraction.
144-172: Text story duplicates ColorSwatch.The Text story renders swatches inline instead of reusing the
ColorSwatchcomponent defined at line 55.export const Text = { render: () => ( <div style={{ padding: '24px', backgroundColor: '#ffffff' }}> <h2 style={{ fontSize: '20px', fontWeight: 600, marginBottom: '16px', color: '#343434' }}> Text Colors </h2> <div style={{ display: 'flex', flexWrap: 'wrap', gap: '16px' }}> {Object.entries(palette.text).map(([name, color]) => ( - <div key={name} style={{ display: 'flex', flexDirection: 'column', alignItems: 'center', gap: '8px' }}> - ... - </div> + <ColorSwatch key={name} name={name} color={color} /> ))} </div> </div> ) };packages/bruno-app/src/components/Dropdown/StyledWrapper.js (1)
140-155: Review the necessity of !important flags.Multiple
!importantdeclarations suggest potential CSS specificity conflicts. While they ensure the active state takes precedence, consider whether the selector specificity could be adjusted to avoid relying on!important.That said, if this resolves existing specificity battles with global styles or Tailwind utilities, the current approach is acceptable.
packages/bruno-app/src/themes/dark/catppuccin-mocha.js (1)
173-173: Consider using color constants for rgba values.Several places use hardcoded rgba values (e.g.,
'rgba(108, 112, 134, 0.16)'on line 173) instead of referencing the defined color constants. For maintainability and consistency, consider using thergbahelper frompolishedwith the color constants:- hoverBg: 'rgba(108, 112, 134, 0.16)', + hoverBg: rgba(colors.OVERLAY0, 0.16),However, if these specific rgba values are intentional overrides or require exact color matching that differs from the named constants, the current approach is acceptable.
Also applies to: 219-219, 382-382, 397-399, 424-424, 499-499, 538-538, 563-563, 576-576, 619-619, 623-623
packages/bruno-app/src/components/Preferences/Themes/index.js (2)
6-21: Hardcoded colors in ThemePreview.The
ThemePreviewcomponent uses hardcoded color values instead of theme tokens. While this might be intentional to show a generic preview, consider whether these should reflect the actual theme colors being previewed.If the preview is meant to be a generic representation, the current implementation is acceptable. However, if it should dynamically reflect the selected theme's actual colors, this would need refactoring.
73-103: Add id attributes for proper label-input association.The radio inputs lack
idattributes, which means the<label>elements are relying on implicit association through wrapping. While this works, explicitid/htmlForassociations improve accessibility and are a best practice.🔎 Proposed enhancement for accessibility
<label className="theme-mode-option"> <input type="radio" + id="theme-mode-light" name="theme-mode" value="light" checked={storedTheme === 'light'} onChange={() => handleModeChange('light')} /> <span>Light</span> </label> <label className="theme-mode-option"> <input type="radio" + id="theme-mode-dark" name="theme-mode" value="dark" checked={storedTheme === 'dark'} onChange={() => handleModeChange('dark')} /> <span>Dark</span> </label> <label className="theme-mode-option"> <input type="radio" + id="theme-mode-system" name="theme-mode" value="system" checked={storedTheme === 'system'} onChange={() => handleModeChange('system')} /> <span>System</span> </label>packages/bruno-app/src/themes/dark/dark-pastel.js (1)
78-82: Consider using named color constants for overlay values.Lines 80-81 use hardcoded hex values
'#555555'and'#444444'instead of named constants from thecolorsobject. For consistency with the rest of the theme (where all colors reference named constants), consider defining these values in thecolorsobject.🔎 Proposed refactor for consistency
const colors = { // Primary palette - glowing pastels against deep purple-black BRAND: '#f0a6ca', // Soft rose - warm and inviting TEXT: '#e8e0f0', // Lavender white - soft, readable TEXT_MUTED: '#a89fc4', // Dusty lavender TEXT_LINK: '#a8c5f0', // Soft periwinkle blue BG: '#1a1625', // Deep plum-black // Core colors WHITE: '#ffffff', BLACK: '#0d0a12', SLATE: '#e8e0f0', // Luminous pastels - glowing against darkness GREEN: '#7dd3a8', // Mint glow YELLOW: '#f0d77d', // Soft gold RED: '#f0887d', // Coral blush PURPLE: '#c4a6f0', // Lavender dream BLUE: '#7db8f0', // Sky shimmer PINK: '#f0a6ca', // Rose petal ORANGE: '#f0b87d', // Peach sunset TEAL: '#7dd3c9', // Aqua glow MAGENTA: '#e09fd9', // Orchid // Deep grayscale with purple undertones GRAY_1: '#1f1a2e', // Deepest plum GRAY_2: '#2a2440', // Dark violet GRAY_3: '#352e4d', // Muted purple GRAY_4: '#453d5c', // Dusty violet GRAY_5: '#5c5478', // Medium violet GRAY_6: '#7a7294', // Soft violet GRAY_7: '#9890ad', // Light violet GRAY_8: '#b8b0cc', // Pale violet GRAY_9: '#d8d0e8', // Whisper violet - GRAY_10: '#f0e8ff', // Lightest violet + GRAY_10: '#f0e8ff', // Lightest violet + OVERLAY_GRAY_1: '#555555', + OVERLAY_GRAY_2: '#444444', // CodeMirror syntax - a constellation of colors CODEMIRROR_TOKENS: { DEFINITION: '#7dd3a8', // Mint - definitions stand out fresh PROPERTY: '#7db8f0', // Sky blue - clear and calm STRING: '#f0a6ca', // Rose - strings feel warm NUMBER: '#7dd3c9', // Teal - numbers are precise ATOM: '#c4a6f0', // Lavender - atoms are special VARIABLE: '#a8c5f0', // Periwinkle - variables flow KEYWORD: '#e09fd9', // Orchid - keywords command attention COMMENT: '#7a7294', // Muted violet - comments recede OPERATOR: '#b8b0cc' // Pale violet - operators connect } }; const darkPastelTheme = { // ... other properties ... overlay: { overlay2: colors.GRAY_6, - overlay1: '#555555', - overlay0: '#444444' + overlay1: colors.OVERLAY_GRAY_1, + overlay0: colors.OVERLAY_GRAY_2 },packages/bruno-app/src/components/StatusBar/ThemeDropdown/index.js (1)
173-173: Simplify onClick handler.The ternary expression
isOpen ? handleClose() : handleOpen()can be simplified by directly toggling based onisOpenor using a single handler function.🔎 Proposed simplification
+ const handleToggle = () => { + if (isOpen) { + handleClose(); + } else { + handleOpen(); + } + }; + return ( <ToolHint text="Theme" toolhintId="ThemeDropdown" place="top" offset={10} hidden={!tooltipEnabled}> <Tippy content={menuContent} placement="top-start" interactive={true} arrow={false} animation={false} visible={isOpen} onClickOutside={handleClose} appendTo="parent" > - <div onClick={() => isOpen ? handleClose() : handleOpen()}> + <div onClick={handleToggle}> {children} </div> </Tippy> </ToolHint> );packages/bruno-app/src/components/RequestPane/WsBody/index.js (1)
16-16: Document or configure the hardcoded feature flag.The
canClientSendMultipleMessagesconstant is hardcoded tofalse. If this is a work-in-progress feature flag, consider:
- Adding a comment explaining why it's disabled and when it will be enabled
- Moving it to a configuration file or feature flags system
- Deriving it from collection/item settings if it should be configurable per request
packages/bruno-app/src/themes/dark/dark.js (1)
2-3: Remove commented-out dead code.Line 2 is a commented duplicate of the BRAND color definition on line 3. This appears to be leftover from development.
🔎 Proposed fix
const colors = { - // BRAND: '#d9a342', BRAND: '#d9a342',
...ages/bruno-app/src/components/CollectionSettings/Overview/RequestsNotLoaded/StyledWrapper.js
Show resolved
Hide resolved
packages/bruno-app/src/components/RequestPane/WsBody/SingleWSMessage/index.js
Show resolved
Hide resolved
...ents/ResponseExample/ResponseExampleResponsePane/ResponseExampleStatusInput/StyledWrapper.js
Show resolved
Hide resolved
packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/GrpcQueryResult/index.js
Show resolved
Hide resolved
...rkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
tests/websockets/persistence.spec.ts (3)
9-13: Extract the duplicated helper to a shared utility.The TODO comment correctly identifies that
isRequestSavedshould be moved to a common location. This function is duplicated intests/request/save/save.spec.ts(lines 6-9) with identical implementation, violating DRY principles.Consider extracting it to
tests/utils/page/helpers.tsor similar. Would you like me to generate the extraction?
58-58: Remove unnecessaryawaiton synchronous assertions.The
awaitkeyword beforeexpect()is redundant here since.toBe()is a synchronous matcher. The pattern should be:-await expect(await isRequestSaved(locators.saveButton())).toBe(false); +expect(await isRequestSaved(locators.saveButton())).toBe(false);And:
-await expect(result).toBe(true); +expect(result).toBe(true);Also applies to: 63-63
37-67: Addtest.stepfor better test reporting.Per coding guidelines,
test.stepshould be promoted "as much as possible" to make generated reports easier to read. Consider breaking down this test into logical steps:🔎 Example refactor with test.step
test('save new websocket url', async ({ pageWithUserData: page }) => { const replacementUrl = 'ws://localhost:8083'; const locators = buildWebsocketCommonLocators(page); const clearText = async (text: string) => { for (let i = text.length; i > 0; i--) { await page.keyboard.press('Backspace'); } }; - await page.locator('#sidebar-collection-name').click(); - await page.getByTitle(BRU_REQ_NAME).click(); + await test.step('Open the websocket request', async () => { + await page.locator('#sidebar-collection-name').click(); + await page.getByTitle(BRU_REQ_NAME).click(); + }); - // remove the original url from the request - await page.locator('.input-container').filter({ hasText: originalUrl }).first().click(); - await clearText(originalUrl); - // replace it with an arbritrary url - await page.keyboard.insertText(replacementUrl); + await test.step('Update websocket URL', async () => { + await page.locator('.input-container').filter({ hasText: originalUrl }).first().click(); + await clearText(originalUrl); + await page.keyboard.insertText(replacementUrl); + }); - // check if the request is now unsaved - await expect(await isRequestSaved(locators.saveButton())).toBe(false); + await test.step('Verify request is marked as unsaved', async () => { + expect(await isRequestSaved(locators.saveButton())).toBe(false); + }); - await locators.saveButton().click(); - const result = await waitForPredicate(() => isRequestSaved(locators.saveButton())); - await expect(result).toBe(true); + await test.step('Save the request', async () => { + await locators.saveButton().click(); + const result = await waitForPredicate(() => isRequestSaved(locators.saveButton())); + expect(result).toBe(true); + }); - // check if the replacementUrl is now visually available - await expect(page.locator('.input-container').filter({ hasText: replacementUrl }).first()).toBeAttached(); + await test.step('Verify new URL is persisted', async () => { + await expect(page.locator('.input-container').filter({ hasText: replacementUrl }).first()).toBeAttached(); + }); });Based on coding guidelines.
tests/request/save/save.spec.ts (3)
6-9: Extract the duplicated helper to a shared utility.This
isRequestSavedfunction is duplicated intests/websockets/persistence.spec.ts(lines 10-13) with identical implementation. Extract to a shared test utility for maintainability.
64-64: Remove unnecessaryawaiton synchronous assertions.The
awaitkeyword beforeexpect()is redundant since.toBe()is synchronous:-await expect(await isRequestSaved(locators.saveButton())).toBe(false); +expect(await isRequestSaved(locators.saveButton())).toBe(false);And:
-await expect(result).toBe(true); +expect(result).toBe(true);Also applies to: 71-71
38-72: Addtest.stepfor better test reporting.Per coding guidelines, wrap test actions in
test.stepblocks to improve report readability. This test has distinct phases (setup, modify URL, verify unsaved, save, verify saved) that would benefit from step organization.🔎 Example refactor with test.step
test('saves new http request', async ({ page, createTmpDir }) => { - // prep the collection by creating a new collection and a new http request - await setup(page, createTmpDir); + await test.step('Setup collection and request', async () => { + await setup(page, createTmpDir); + }); const locators = buildCommonLocators(page); const originalUrl = 'https://echo.usebruno.com'; const replacementUrl = 'ws://localhost:8082'; const clearText = async (text: string) => { for (let i = text.length; i > 0; i--) { await page.keyboard.press('Backspace'); } }; - // Open the request tab - await page.locator('.collection-item-name').filter({ hasText: 'test-request' }).dblclick(); - await expect(page.locator('.request-tab .tab-label').filter({ hasText: 'test-request' })).toBeVisible(); + await test.step('Open request tab', async () => { + await page.locator('.collection-item-name').filter({ hasText: 'test-request' }).dblclick(); + await expect(page.locator('.request-tab .tab-label').filter({ hasText: 'test-request' })).toBeVisible(); + }); - // remove the original url from the request - await page.locator('.input-container').filter({ hasText: originalUrl }).first().click(); - await clearText(originalUrl); - // replace it with an arbitrary url - await page.keyboard.insertText(replacementUrl); + await test.step('Modify request URL', async () => { + await page.locator('.input-container').filter({ hasText: originalUrl }).first().click(); + await clearText(originalUrl); + await page.keyboard.insertText(replacementUrl); + }); - // check if the request is now unsaved - await expect(await isRequestSaved(locators.saveButton())).toBe(false); + await test.step('Verify request is marked as unsaved', async () => { + expect(await isRequestSaved(locators.saveButton())).toBe(false); + }); - // trigger a save - locators.saveButton().click(); - // Wait for it to be saved - const result = await waitForPredicate(() => isRequestSaved(locators.saveButton())); - await expect(result).toBe(true); + await test.step('Save and verify persistence', async () => { + locators.saveButton().click(); + const result = await waitForPredicate(() => isRequestSaved(locators.saveButton())); + expect(result).toBe(true); + }); });Based on coding guidelines.
packages/bruno-app/src/themes/index.js (2)
32-98: Consider deriving themeRegistry from theme objects to reduce duplication.The themeRegistry duplicates all theme IDs from the
themesobject, creating a maintenance burden. If a theme is added or removed, both structures must be manually synchronized.🔎 Proposed refactor to derive metadata from theme objects
Each theme object could include its own metadata (id, name, mode), then
themeRegistrycan be derived:+// In each theme file (e.g., light/light.js), add metadata: +const lightTheme = { + id: 'light', + name: 'Bruno Light', + mode: 'light', + brand: colors.BRAND, + // ... rest of theme +}; +// Then derive themeRegistry: -export const themeRegistry = { - 'light': { - id: 'light', - name: 'Bruno Light', - mode: 'light' - }, - // ... all other themes -}; +export const themeRegistry = Object.keys(themes).reduce((registry, key) => { + const theme = themes[key]; + registry[key] = { + id: theme.id || key, + name: theme.name, + mode: theme.mode + }; + return registry; +}, {});This ensures single source of truth and reduces the chance of inconsistencies.
100-101: Consider memoizing the theme filter results.The helper functions recreate arrays on every call. If these are called frequently, consider computing the results once or memoizing them.
🔎 Optional memoization approach
-export const getLightThemes = () => Object.values(themeRegistry).filter((t) => t.mode === 'light'); -export const getDarkThemes = () => Object.values(themeRegistry).filter((t) => t.mode === 'dark'); +const lightThemes = Object.values(themeRegistry).filter((t) => t.mode === 'light'); +const darkThemes = Object.values(themeRegistry).filter((t) => t.mode === 'dark'); + +export const getLightThemes = () => lightThemes; +export const getDarkThemes = () => darkThemes;This avoids recreating arrays on every call, though the performance gain is minimal unless called very frequently.
packages/bruno-app/src/themes/light/light-monochrome.js (1)
1-36: Semantic color naming is misleading in monochrome context.Color constants like
GREEN(line 11) andYELLOW(line 12) are defined as gray values (#525252), which creates semantic confusion. While this achieves the monochrome effect, developers might expect these constants to represent actual green and yellow colors.🔎 Proposed semantic improvement
Consider using semantically neutral names for grayscale values:
const colors = { BRAND: '#525252', TEXT: 'rgb(52, 52, 52)', TEXT_MUTED: '#737373', TEXT_LINK: '#404040', BACKGROUND: '#fff', WHITE: '#fff', BLACK: '#000', SLATE_BLACK: '#343434', - GREEN: '#525252', - YELLOW: '#525252', + ACCENT_PRIMARY: '#525252', + ACCENT_SECONDARY: '#525252', // ... and update references below - yellow: colors.YELLOW, + yellow: colors.ACCENT_SECONDARY,This makes the intent clearer and avoids confusion about expected color values.
packages/bruno-app/src/components/RequestPane/GrpcBody/index.js (1)
186-186: Consider optional chaining for defensive programming.While
body.grpc.lengthis currently safe due to the parent guard (line 245), adding optional chaining would make this more resilient to future refactoring:- const isSingleMessage = !canClientSendMultipleMessages || body.grpc.length === 1; + const isSingleMessage = !canClientSendMultipleMessages || body.grpc?.length === 1;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/bruno-app/src/components/Preferences/index.jspackages/bruno-app/src/components/RequestPane/GrpcBody/index.jspackages/bruno-app/src/components/TagList/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/themes/index.jspackages/bruno-app/src/themes/light/light-monochrome.jspackages/bruno-app/src/themes/light/light.jstests/grpc/make-request/make-request.spec.tstests/request/save/save.spec.tstests/utils/page/locators.tstests/websockets/persistence.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/components/Preferences/index.js
- packages/bruno-app/src/themes/light/light.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/GrpcBody/index.jstests/grpc/make-request/make-request.spec.tstests/websockets/persistence.spec.tspackages/bruno-app/src/themes/index.jstests/utils/page/locators.tstests/request/save/save.spec.tspackages/bruno-app/src/themes/light/light-monochrome.jspackages/bruno-app/src/components/TagList/StyledWrapper.js
tests/**/**.*
⚙️ CodeRabbit configuration file
tests/**/**.*: Review the following e2e test code written using the Playwright test library. Ensure that:
Follow best practices for Playwright code and e2e automation
Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright callsAvoid using
page.pause()in codeUse locator variables for locators
Avoid using test.only
Use multiple assertions
Promote the use of
test.stepas much as possible so the generated reports are easier to readEnsure that the
fixtureslike the collections are nested inside thefixturesfolderFixture Example*: Here's an example of possible fixture and test pair
. ├── fixtures │ └── collection │ ├── base.bru │ ├── bruno.json │ ├── collection.bru │ ├── ws-test-request-with-headers.bru │ ├── ws-test-request-with-subproto.bru │ └── ws-test-request.bru ├── connection.spec.ts # <- Depends on the collection in ./fixtures/collection ├── headers.spec.ts ├── persistence.spec.ts ├── variable-interpolation │ ├── fixtures │ │ └── collection │ │ ├── environments │ │ ├── bruno.json │ │ └── ws-interpolation-test.bru │ ├── init-user-data │ └── variable-interpolation.spec.ts # <- Depends on the collection in ./variable-interpolation/fixtures/collection └── subproto.spec.ts
Files:
tests/grpc/make-request/make-request.spec.tstests/websockets/persistence.spec.tstests/utils/page/locators.tstests/request/save/save.spec.ts
🧠 Learnings (4)
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.{jsx,tsx} : Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component or any React component using the styled component
Applied to files:
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/GrpcBody/index.jspackages/bruno-app/src/themes/index.jspackages/bruno-app/src/themes/light/light-monochrome.jspackages/bruno-app/src/components/TagList/StyledWrapper.js
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.{jsx,tsx} : Styled Component CSS might also change layout but Tailwind classes shouldn't define colors
Applied to files:
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/TagList/StyledWrapper.js
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.{jsx,tsx} : Styled Components are used as wrappers to define both self and children components style; Tailwind classes are used specifically for layout based styles
Applied to files:
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/TagList/StyledWrapper.js
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/GrpcBody/index.jspackages/bruno-app/src/themes/index.jspackages/bruno-app/src/themes/light/light-monochrome.jspackages/bruno-app/src/components/TagList/StyledWrapper.js
🧬 Code graph analysis (3)
packages/bruno-app/src/components/RequestPane/GrpcBody/index.js (1)
packages/bruno-app/src/ui/Button/index.js (1)
Button(4-79)
tests/websockets/persistence.spec.ts (1)
tests/utils/wait.ts (1)
waitForPredicate(4-13)
packages/bruno-app/src/themes/light/light-monochrome.js (1)
packages/bruno-app/src/themes/light/light.js (1)
colors(52-64)
⏰ 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: Unit Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (10)
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js (1)
12-12: LGTM! Border declarations are now consistent.All border declarations correctly use the
solid 1pxprefix withtheme.border.border0, resolving the previous inconsistency. The theming refactor is well-executed and uniform across the table structure.Also applies to: 48-49, 66-67
packages/bruno-app/src/components/TagList/StyledWrapper.js (1)
29-30: LGTM — improved semantic clarity.The theme token changes align well with the theming overhaul. Moving from
requestTabs.active.bgtobackground.surface2is semantically clearer for tag styling, and the border color now consistently matches the base state (line 18).tests/grpc/make-request/make-request.spec.ts (1)
91-91: LGTM! Locator update aligns with UI refactor.The update from
accordion()tolist()is consistent with the locator definition change and properly reflects the UI component refactor from accordion to list.Also applies to: 193-193
tests/utils/page/locators.ts (1)
198-198: Locator rename verified and complete.The refactor from
accordiontolistis clean with no orphaned references. The new test IDgrpc-responses-listis properly implemented in the component code atpackages/bruno-app/src/components/ResponsePane/GrpcResponsePane/GrpcQueryResult/index.js:66, and all references have been updated consistently.packages/bruno-app/src/themes/index.js (1)
1-29: LGTM!The theme imports and registration structure is clean and follows coding standards. The mix of plain identifiers and kebab-case string keys appropriately handles the different theme naming conventions.
packages/bruno-app/src/themes/light/light-monochrome.js (2)
38-92: LGTM!The theme object is properly renamed to
lightMonochromeThemewithmode: 'light'metadata. The new structure withaccents,background, andoverlaysections provides good organization for the monochrome palette.
418-617: LGTM!The remaining component theme definitions consistently apply the monochrome palette throughout. The export statement correctly uses
lightMonochromeTheme. All coding standards are followed.packages/bruno-app/src/components/RequestPane/GrpcBody/index.js (3)
245-262: LGTM—Empty state Button usage is consistent.The empty state correctly uses the UI
Buttoncomponent with appropriate props, aligning with the PR's component standardization objective.
225-230: LGTM—Auto-scroll behavior is implemented correctly.The useEffect properly scrolls to the latest message when new messages are added, with the correct dependency array.
264-286: LGTM—Message filtering and rendering logic is correct.The
messagesToShowfilter correctly handles both single and multiple message scenarios. The mapping indices align with the originalbody.grpcarray indices because the filter only ever removes messages from position 1 onward (whencanClientSendMultipleMessagesis false), preserving the index correspondence for operations likeonEditandonDeleteMessage.
| const MessageToolbar = ({ | ||
| index, | ||
| canClientStream, | ||
| isConnectionActive, | ||
| onSend, | ||
| onRegenerateMessage, | ||
| onPrettify, | ||
| onDeleteMessage, | ||
| showDelete | ||
| }) => { | ||
| return ( | ||
| <div className="message-toolbar"> | ||
| <span className="message-label">Message {index + 1}</span> | ||
| <div className="toolbar-actions mr-2"> | ||
| <ToolHint text="Format JSON" toolhintId={`prettify-msg-${index}`}> | ||
| <button onClick={onPrettify} className="toolbar-btn"> | ||
| <IconWand size={16} strokeWidth={1.5} /> | ||
| </button> | ||
| </ToolHint> | ||
|
|
||
| <ToolHint text="Generate sample" toolhintId={`regenerate-msg-${index}`}> | ||
| <button onClick={onRegenerateMessage} className="toolbar-btn"> | ||
| <IconRefresh size={16} strokeWidth={1.5} /> | ||
| </button> | ||
| </ToolHint> | ||
|
|
||
| {canClientStream && ( | ||
| <ToolHint text={isConnectionActive ? 'Send message' : 'Connection not active'} toolhintId={`send-msg-${index}`}> | ||
| <button | ||
| onClick={onSend} | ||
| disabled={!isConnectionActive} | ||
| className={`toolbar-btn ${!isConnectionActive ? 'disabled' : ''}`} | ||
| data-testid={`grpc-send-message-${index}`} | ||
| > | ||
| <IconSend size={16} strokeWidth={1.5} /> | ||
| </button> | ||
| </ToolHint> | ||
| )} | ||
|
|
||
| {showDelete && ( | ||
| <ToolHint text="Delete message" toolhintId={`delete-msg-${index}`}> | ||
| <button onClick={onDeleteMessage} className="toolbar-btn delete"> | ||
| <IconTrash size={16} strokeWidth={1.5} /> | ||
| </button> | ||
| </ToolHint> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Replace native buttons with the UI Button component for consistency.
The MessageToolbar uses native <button> elements (lines 35, 41, 48, 61), which contradicts the PR objective of "replacing native buttons with a shared Button component." The empty state (line 250) and footer (line 290) correctly use the Button component—the toolbar should follow the same pattern.
🔎 Example refactor for the prettify button
- <ToolHint text="Format JSON" toolhintId={`prettify-msg-${index}`}>
- <button onClick={onPrettify} className="toolbar-btn">
- <IconWand size={16} strokeWidth={1.5} />
- </button>
- </ToolHint>
+ <ToolHint text="Format JSON" toolhintId={`prettify-msg-${index}`}>
+ <Button
+ onClick={onPrettify}
+ variant="ghost"
+ size="sm"
+ icon={<IconWand size={16} strokeWidth={1.5} />}
+ />
+ </ToolHint>Apply the same pattern to the other three buttons (regenerate, send, delete).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/bruno-app/src/components/RequestPane/GrpcBody/index.js around lines
20 to 69, replace the native <button> elements used for prettify, regenerate,
send and delete with the shared Button component to match the rest of the PR:
import the Button component at the top, then for each ToolHint child use
<Button> with the same onClick handlers (onPrettify, onRegenerateMessage,
onSend, onDeleteMessage), pass disabled for the send button when
!isConnectionActive, preserve className="toolbar-btn" (and "delete" for the
delete button) and data-testid={`grpc-send-message-${index}`}, and keep the icon
children exactly as-is so visual/behavioral parity is maintained. Ensure the
send button still applies the conditional disabled state and styling and that
all buttons keep their original aria/test ids and props.
Custom Themes + UX Overhaul
Summary by CodeRabbit
Release Notes
New Features
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.