packages/bruno-app/src/components/Sidebar/SidebarHeader/index.js (1)

171-221: Simplify onClick handlers in dropdown item configs.

The arrow function wrappers are unnecessary when the handler takes no arguments.

   const addDropdownItems = [
     {
       id: 'create',
       leftSection: IconPlus,
       label: 'Create collection',
-      onClick: () => {
-        setCreateCollectionModalOpen(true);
-      }
+      onClick: () => setCreateCollectionModalOpen(true)
     },
     {
       id: 'import',
       leftSection: IconDownload,
       label: 'Import collection',
-      onClick: () => {
-        setImportCollectionModalOpen(true);
-      }
+      onClick: () => setImportCollectionModalOpen(true)
     },
     {
       id: 'open',
       leftSection: IconFolder,
       label: 'Open collection',
-      onClick: () => {
-        handleOpenCollection();
-      }
+      onClick: handleOpenCollection
     }
   ];

   const actionsDropdownItems = [
     {
       id: 'sort',
       leftSection: getSortIcon(),
       label: getSortLabel(),
-      onClick: () => {
-        handleSortCollections();
-      },
+      onClick: handleSortCollections,
       title: 'Sort collections',
       testId: 'sort-collections-button'
     },
     {
       id: 'close-all',
       leftSection: IconSquareX,
       label: 'Close all',
-      onClick: () => {
-        selectAllCollectionsToClose();
-      },
+      onClick: selectAllCollectionsToClose,
       title: 'Close all collections',
       testId: 'close-all-collections-button'
     }
   ];

packages/bruno-app/src/ui/MenuDropdown/index.js (2)

1-2: Consolidate React imports.

Minor nit: combine the React imports into a single statement.

-import React from 'react';
-import { useRef, useCallback, useState } from 'react';
+import React, { useRef, useCallback, useState } from 'react';

---

256-272: Unused itemIndex variable.

itemIndex is declared but only incremented for dividers. Since dividers can use the map index directly, this variable is unnecessary.

   const renderMenuContent = () => {
-    let itemIndex = 0;
-
-    return items.map((item) => {
+    return items.map((item, index) => {
       const itemType = item.type || 'item';

       if (itemType === 'label') {
         return renderLabel(item);
       }

       if (itemType === 'divider') {
-        return renderDivider(item, itemIndex++);
+        return renderDivider(item, index);
       }

       return renderMenuItem(item);
     });
   };

packages/bruno-app/src/components/AppTitleBar/index.js (1)

59-66: Move WorkspaceName outside the component or memoize it.

Defining forwardRef components inside render creates a new component type each render, causing React to unmount/remount it unnecessarily.

Move it outside AppTitleBar:

+const WorkspaceName = forwardRef(({ workspaceName, ...props }, ref) => {
+  return (
+    <div ref={ref} className="workspace-name-container" {...props}>
+      <span className="workspace-name">{workspaceName || 'Default Workspace'}</span>
+      <IconChevronDown size={14} stroke={1.5} className="chevron-icon" />
+    </div>
+  );
+});

 const AppTitleBar = () => {
   // ...
-  const WorkspaceName = forwardRef((props, ref) => {
-    return (
-      <div ref={ref} className="workspace-name-container" {...props}>
-        <span className="workspace-name">{toTitleCase(activeWorkspace?.name) || 'Default Workspace'}</span>
-        <IconChevronDown size={14} stroke={1.5} className="chevron-icon" />
-      </div>
-    );
-  });

Then update usage:

<WorkspaceName workspaceName={toTitleCase(activeWorkspace?.name)} />

packages/bruno-app/src/ui/ActionIcon/StyledWrapper.js (1)

244-264: Consider the specificity implications of !important.

The !important declarations in colorOnHover ensure the hover color always applies (useful for destructive actions), but they make the styles harder to override in edge cases. If this is the intended behavior, this is acceptable.

packages/bruno-app/src/ui/ActionIcon/index.js (1)

4-19: Minor JSDoc clarification.

Line 16 documents ariaLabel but the actual destructured prop is 'aria-label' (line 29). For clarity, consider documenting it as @param {string} props['aria-label'] or adding a note that it maps to the standard aria-label attribute.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:32.975Z
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

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:32.975Z
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

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:32.975Z
Learning: Applies to **/*.{jsx,tsx} : Styled Component CSS might also change layout but Tailwind classes shouldn't define colors

packages/bruno-app/src/ui/MenuDropdown/index.js (2)

145-167: Consider alternatives to setTimeout(0) for focus management.

The setTimeout(0) pattern works but is fragile and depends on the browser's event loop timing. While this is a common pattern for deferring operations until the DOM is ready, it can occasionally fail if rendering takes longer than expected.

Consider using requestAnimationFrame for more reliable timing, or checking element readiness before focusing:

-        onShow: () => {
-          // Focus selected item if available, otherwise focus menu container
-          setTimeout(() => {
-            const menuContainer = ref.popper?.querySelector('[role="menu"]');
-            if (!menuContainer) return;
-
-            // If selectedItemId is provided, find and focus that item
-            if (selectedItemId) {
+        onShow: () => {
+          // Focus selected item if available, otherwise focus menu container
+          requestAnimationFrame(() => {
+            const menuContainer = ref.popper?.querySelector('[role="menu"]');
+            if (!menuContainer) return;
+
+            // If selectedItemId is provided, find and focus that item
+            if (selectedItemId) {
               const menuItems = Array.from(
                 menuContainer.querySelectorAll('[role="menuitem"]:not([aria-disabled="true"])')
               );
-
+
               const selectedItem = menuItems.find(
                 (item) => item.getAttribute('data-item-id') === selectedItemId
               );
-
+
               if (selectedItem) {
                 focusMenuItem(selectedItem, true);
                 return;
               }
             }
-
+
             // Fallback: focus menu container
             menuContainer.focus();
-          }, 0);
+          });
         }

---

206-216: Document rightSection click isolation behavior.

The click handler on the rightSection wrapper prevents event propagation to the parent menuitem, which allows interactive elements in the rightSection without triggering the menu item action. This is likely intentional but isn't documented in the JSDoc or inline comments.

Add an inline comment explaining this behavior:

         {item.rightSection && (
           <div
             className="dropdown-right-section"
+            // Prevent clicks on rightSection from triggering the menu item action
             onClick={(e) => {
               e.preventDefault();
               e.stopPropagation();
             }}
           >

Additionally, update the JSDoc to clarify that rightSection elements can be interactive without triggering the menu item click.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

packages/bruno-app/src/ui/MenuDropdown/index.js (1)

187-217: Add aria-label fallback for accessibility.

The aria-label only uses item.ariaLabel directly. If a consumer provides only label or title, the accessibility label will be undefined. This was flagged in a prior review.

Apply this diff to add proper fallback:

         aria-label={item.ariaLabel}
+        aria-label={item.ariaLabel || item.label || item.title}

Also, if ariaLabel is undefined and no fallback is provided, consider omitting the attribute entirely to avoid rendering aria-label="":

-        aria-label={item.ariaLabel}
+        aria-label={item.ariaLabel || item.label || item.title || undefined}

packages/bruno-app/src/ui/ActionIcon/StyledWrapper.js (1)

22-51: Consider validating the $size prop fallback.

Lines 32-33 fall back to props.$size directly if it's not in sizeMap. This allows custom numeric sizes but could produce invalid CSS (e.g., undefinedpx or invalidpx) if an unexpected value is passed.

Consider adding a safer fallback:

-  width: ${(props) => sizeMap[props.$size] || props.$size}px;
-  height: ${(props) => sizeMap[props.$size] || props.$size}px;
+  width: ${(props) => sizeMap[props.$size] || props.$size || sizeMap.md}px;
+  height: ${(props) => sizeMap[props.$size] || props.$size || sizeMap.md}px;

packages/bruno-app/src/ui/MenuDropdown/index.js (2)

1-2: Consolidate React imports.

Minor style nit: these can be combined into a single import statement.

-import React from 'react';
-import { useRef, useCallback, useState } from 'react';
+import React, { useRef, useCallback, useState } from 'react';

---

71-88: Wrap focusMenuItem in useCallback.

This function is referenced inside handleMenuKeyDown and onDropdownCreate but isn't memoized. This could cause unnecessary re-renders or stale closure issues. ESLint's exhaustive-deps rule would likely flag this.

-  // Focus a menu item
-  const focusMenuItem = (item, addSelectedClass = false) => {
+  // Focus a menu item
+  const focusMenuItem = useCallback((item, addSelectedClass = false) => {
     if (item) {
       // Remove selected class from all items first
       const menuContainer = item.closest('[role="menu"]');
       if (menuContainer) {
         menuContainer.querySelectorAll('.selected-focused').forEach((el) => {
           el.classList.remove('selected-focused');
         });
       }

       if (addSelectedClass) {
         item.classList.add('selected-focused');
       }
       item.focus();
       item.scrollIntoView({ block: 'nearest' });
     }
-  };
+  }, []);

Then add focusMenuItem to the dependency arrays of handleMenuKeyDown (line 126) and onDropdownCreate (line 171).

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:32.975Z
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

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:32.975Z
Learning: Applies to **/*.{jsx,tsx} : Styled Component CSS might also change layout but Tailwind classes shouldn't define colors

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:32.975Z
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

packages/bruno-app/src/components/Sidebar/SidebarHeader/index.js (3)

171-219: Dropdown item configuration is clear and correctly wired

The addDropdownItems and actionsDropdownItems arrays are well-structured: ids, icons, labels, and handlers are all consistent, and the sort label/icon correctly represent the next sort state, not the current one. If you ever see unnecessary re-renders in MenuDropdown, these arrays would be good candidates for useMemo, but that’s an optional micro-optimization for now.

---

224-257: ActionIcon + MenuDropdown wiring for collections actions is solid

Search, add, and “more actions” are all correctly routed through existing handlers, and the ActionIcon labels plus aria-hidden icons give you good accessibility semantics. If you later surface a “search is open” boolean from the parent, it may be worth threading that into ActionIcon (e.g., as an active/aria-pressed-like state), but that’s purely a UX refinement.

---

272-305: ARIA updates for the view switcher improve semantics; consider full tabs pattern

Using role="heading" for the single view and tablist/tab/aria-selected on the multi-view buttons is a nice accessibility upgrade, and icons being aria-hidden avoids duplicate announcements. If the associated panels aren’t already wired elsewhere, you may want to pair each tab with a corresponding role="tabpanel" element whose id matches aria-controls to fully align with the WAI-ARIA tabs pattern.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

packages/bruno-app/src/ui/MenuDropdown/index.js (1)

18-39: Align item label / aria-label / title behavior with JSDoc and A11y expectations

The JSDoc says label is used for both title and aria-label (preferred) and that ariaLabel/title fall back accordingly, but renderMenuItem currently wires these independently:

• Visible text: item.label
• aria-label: item.ariaLabel
• title: item.title

So items with only label don’t get an explicit aria-label/title, and items with only title don’t expose that to A11y unless the text content matches.

To match the docs and prioritize A11y overrides, consider resolving once and reusing:

-  const renderMenuItem = (item) => {
-    return (
+  const renderMenuItem = (item) => {
+    const resolvedLabel = item.ariaLabel || item.label || item.title;
+    const resolvedTitle = item.title || resolvedLabel;
+
+    return (
       <div
         key={item.id}
         className={`dropdown-item ${item.disabled ? 'disabled' : ''} ${item.className || ''}`.trim()}
         role="menuitem"
         data-item-id={item.id}
         onClick={() => !item.disabled && handleItemClick(item)}
         tabIndex={item.disabled ? -1 : 0}
-        aria-label={item.ariaLabel}
+        aria-label={resolvedLabel}
         aria-disabled={item.disabled}
-        title={item.title}
+        title={resolvedTitle}
         data-testid={item.testId}
       >
         {renderSection(item.leftSection)}
-        <span className="dropdown-label">{item.label}</span>
+        <span className="dropdown-label">{item.label || resolvedLabel}</span>
         ...
       </div>
-    );
+    );
   };

This gives ariaLabel highest priority, falls back to label/title, and keeps the docs accurate.

Also applies to: 187-201

packages/bruno-app/src/ui/ActionIcon/index.js (1)

35-48: Default ActionIcon’s native button to type="button"

When component is left as the default 'button', the underlying element will be a submit button inside forms, which can cause unexpected submissions.

You can guard against this by defaulting the type when rendering a native button:

 const ActionIcon = ({
   ...
   component: Component = 'button',
   ...
   colorOnHover,
-  ...rest
+  ...rest
 }) => {
   ...
-  return (
-    <StyledWrapper
-      as={Component}
+  const { type, ...restProps } = rest;
+
+  return (
+    <StyledWrapper
+      as={Component}
       $variant={variant}
       $size={size}
       $colorOnHover={colorOnHover}
       disabled={disabled}
       className={classNames}
-      title={label}
-      aria-label={ariaLabel}
-      {...rest}
+      {...(Component === 'button' ? { type: type || 'button' } : {})}
+      {...restProps}
     >
       {children}
     </StyledWrapper>
   );
 };

This keeps existing behavior for non-button components while making the default safe inside forms.

packages/bruno-app/src/components/AppTitleBar/index.js (1)

59-67: Tidy WorkspaceName definition and workspaceMenuItems dependencies

Functionally this works, but a couple of small refactors would make it cleaner and more lint-friendly:

• WorkspaceName is created with forwardRef inside AppTitleBar, so a new component type is allocated every render. Moving it outside the component (or wrapping the forwardRef call in a useMemo([])) would stabilize its identity.
• workspaceMenuItems’s useMemo depends on sortedWorkspaces, activeWorkspaceUid, preferences, and handlePinWorkspace, but it also closes over handleWorkspaceSwitch, handleCreateWorkspace, and handleOpenWorkspace. They’re stable enough in practice, but strict hook linting would either ask to add them as dependencies (or wrap them in useCallback) or to drop useMemo if the optimization isn’t needed.

None of this is blocking, but worth tightening up to keep hook rules and component identity straightforward.

Also applies to: 123-175

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

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

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

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

packages/bruno-app/src/ui/MenuDropdown/index.js (1)

228-230: Divider class name may still not match StyledWrapper’s styles

The divider is rendered with className="dropdown-divider". Previous styling in components/Dropdown/StyledWrapper.js targeted .dropdown-separator, and an earlier review already flagged this mismatch.

Unless the styles were updated to also cover .dropdown-divider, this separator still won’t pick up the intended visuals. Either align the class name here:

-  const renderDivider = (item, index) => (
-    <div key={item.id || `divider-${index}`} className="dropdown-divider" role="separator" />
-  );
+  const renderDivider = (item, index) => (
+    <div key={item.id || `divider-${index}`} className="dropdown-separator" role="separator" />
+  );

or alias both class names in the StyledWrapper so they share the same styling.

packages/bruno-app/src/components/Sidebar/SidebarHeader/index.js (1)

181-249: Dropdown item configs look good; consider memoizing for stability

addDropdownItems and actionsDropdownItems are rebuilt on every render, even though they only depend on stable handlers and Redux state. This is fine for now, but if the header ever re-renders frequently it may cause extra work for MenuDropdown reconciliation.

You could wrap these arrays in useMemo keyed on the minimal state they depend on (e.g. collectionSortOrder) to keep the item identities stable and reduce render churn, especially if tests rely on referential equality.

packages/bruno-app/src/components/AppTitleBar/index.js (2)

59-63: Preserve default workspace-name-container class when spreading props

Spreading props after className="workspace-name-container" means any incoming className will override the default, potentially dropping required styling if the component is reused elsewhere.

To make this more robust, consider explicitly merging classes instead of overwriting:

-  const WorkspaceName = forwardRef((props, ref) => {
-    return (
-      <div ref={ref} className="workspace-name-container" {...props}>
+  const WorkspaceName = forwardRef(({ className = '', ...rest }, ref) => {
+    return (
+      <div
+        ref={ref}
+        className={`workspace-name-container ${className}`.trim()}
+        {...rest}
+      >
         <span className="workspace-name">{toTitleCase(activeWorkspace?.name) || 'Default Workspace'}</span>
         <IconChevronDown size={14} stroke={1.5} className="chevron-icon" />
       </div>
     );
   });

---

123-174: Ensure workspaceMenuItems useMemo stays aligned with hooks lint and handlers

The workspaceMenuItems construction is solid (active checks, pin/unpin right section, workspace actions appended), but the useMemo dependency list omits the handler functions it closes over (handleWorkspaceSwitch, handleCreateWorkspace, handleOpenWorkspace).

To avoid future surprises and satisfy exhaustive-deps lint, either:

• Wrap those handlers in useCallback and include them in the dependency array, or
• Drop useMemo and build the menu inline if the workspace list is small enough that memoization isn’t buying much.

For example (one option):

-  const workspaceMenuItems = useMemo(() => {
+  const workspaceMenuItems = useMemo(() => {
     const items = sortedWorkspaces.map((workspace) => {
       const isActive = workspace.uid === activeWorkspaceUid;
       const isPinned = preferences?.workspaces?.pinnedWorkspaceUids?.includes(workspace.uid);
@@
     items.push(
       { type: 'label', label: 'Workspaces' },
       {
         id: 'create-workspace',
         leftSection: IconPlus,
         label: 'Create workspace',
         onClick: handleCreateWorkspace
       },
       {
         id: 'open-workspace',
         leftSection: IconFolder,
         label: 'Open workspace',
         onClick: handleOpenWorkspace
       }
     );
 
     return items;
-  }, [sortedWorkspaces, activeWorkspaceUid, preferences, handlePinWorkspace]);
+  }, [sortedWorkspaces, activeWorkspaceUid, preferences, handlePinWorkspace, handleWorkspaceSwitch, handleCreateWorkspace, handleOpenWorkspace]);

packages/bruno-app/src/ui/MenuDropdown/index.js (1)

6-16: Minor cleanup: remove Escape from NAVIGATION_KEYS to reflect actual behavior

Escape is included in NAVIGATION_KEYS, but it is handled separately and returns early in handleMenuKeyDown. As a result, it never reaches the navigation block, and having it in NAVIGATION_KEYS is slightly misleading.

You can simplify the intent by dropping it from the navigation list:

-const NAVIGATION_KEYS = ['ArrowDown', 'ArrowUp', 'Home', 'End', 'Escape'];
+const NAVIGATION_KEYS = ['ArrowDown', 'ArrowUp', 'Home', 'End'];

This keeps the key groups self-describing without changing behavior.

Also applies to: 91-127

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : No trailing commas

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

packages/bruno-app/src/ui/MenuDropdown/index.js (2)

227-230: Separator class name may not match styles.

The divider uses className="dropdown-divider" but styled components may expect .dropdown-separator. Verify the styling is applied correctly or align the class name.

#!/bin/bash
# Check what separator class names are used in styled components
rg -n "dropdown-separator|dropdown-divider" --type=js

---

189-218: Add fallback for aria-label accessibility.

Line 198 only uses item.ariaLabel, but if it's undefined, the element won't have an accessible name. Add a fallback chain for better accessibility:

         tabIndex={item.disabled ? -1 : 0}
-        aria-label={item.ariaLabel}
+        aria-label={item.ariaLabel || item.label || item.title}
         aria-disabled={item.disabled}

packages/bruno-app/src/components/Sidebar/SidebarHeader/index.js (1)

181-227: Dropdown item configuration looks clean.

The declarative item structure for addDropdownItems is well-organized. Each item has a unique id, appropriate icon via leftSection, and onClick handlers.

One minor inconsistency: the onClick handlers use arrow functions wrapping single function calls (e.g., onClick: () => { handleOpenCollection(); }). These could be simplified.

     {
       id: 'open',
       leftSection: IconFolder,
       label: 'Open collection',
-      onClick: () => {
-        handleOpenCollection();
-      }
+      onClick: handleOpenCollection
     },

Apply similar simplification to other items where the handler is a direct function reference.

packages/bruno-app/src/components/AppTitleBar/index.js (1)

1-3: Consolidate React imports.

Lines 1 and 3 both import from 'react'. Consider consolidating:

-import React from 'react';
-import { IconCheck, IconChevronDown, IconFolder, IconHome, IconLayoutColumns, IconLayoutRows, IconPin, IconPinned, IconPlus } from '@tabler/icons';
-import { forwardRef, useCallback, useEffect, useMemo, useState } from 'react';
+import React, { forwardRef, useCallback, useEffect, useMemo, useState } from 'react';
+import { IconCheck, IconChevronDown, IconFolder, IconHome, IconLayoutColumns, IconLayoutRows, IconPin, IconPinned, IconPlus } from '@tabler/icons';

packages/bruno-app/src/ui/MenuDropdown/index.js (1)

1-2: Consolidate React imports.

Same pattern as AppTitleBar - consolidate the imports:

-import React from 'react';
-import { useRef, useCallback, useState } from 'react';
+import React, { useRef, useCallback, useState } from 'react';

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : No trailing commas

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

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

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

packages/bruno-app/src/ui/MenuDropdown/index.js (1)

139-172: selectedItemId focus behavior will be stale after initial mount

Because onDropdownCreate only runs when Tippy creates the instance, the onShow handler it registers captures whatever selectedItemId existed at that first render. Later prop changes won’t update the focus target, so menus may focus the wrong item when selectedItemId changes (e.g., active workspace switch).

You can keep focus behavior in sync by storing the ref in onDropdownCreate and moving the setProps({ onShow }) logic into an effect that depends on selectedItemId (and a memoized focusMenuItem):

-import { useRef, useCallback, useState } from 'react';
+import { useRef, useCallback, useState, useEffect } from 'react';
@@
-  const focusMenuItem = (item, addSelectedClass = false) => {
+  const focusMenuItem = useCallback((item, addSelectedClass = false) => {
     if (item) {
@@
-      item.scrollIntoView({ block: 'nearest' });
+      item.scrollIntoView({ block: 'nearest' });
     }
-  };
+  }, []);
@@
-  const onDropdownCreate = useCallback((ref) => {
-    tippyRef.current = ref;
-    if (ref) {
-      ref.setProps({
-        onShow: () => {
-          // Focus selected item if available, otherwise focus menu container
-          setTimeout(() => {
-            const menuContainer = ref.popper?.querySelector('[role="menu"]');
+  const onDropdownCreate = useCallback((ref) => {
+    tippyRef.current = ref;
+  }, []);
+
+  useEffect(() => {
+    const ref = tippyRef.current;
+    if (!ref) return;
+
+    ref.setProps({
+      onShow: () => {
+        setTimeout(() => {
+          const menuContainer = ref.popper?.querySelector('[role="menu"]');
             if (!menuContainer) return;
@@
-            menuContainer.focus();
-          }, 0);
-        }
-      });
-    }
-  }, [selectedItemId]);
+            menuContainer.focus();
+          }, 0);
+      }
+    });
+  }, [selectedItemId, focusMenuItem]);

This keeps the open-focus behavior aligned with the latest selectedItemId without recreating the Tippy instance.

tests/import/wsdl/import-wsdl.spec.ts (1)

73-80: Optional: de-duplicate the “open import collection modal” interaction

This step is identical to the one in the XML test above. Consider pulling the “open import collection modal” interaction (including the collections-header-add-menu locator) into a small helper so future UI tweaks only need to be updated in one place. Not required for this PR, but it would reduce duplication and keep locators centralized.

tests/request/encoding/curl-encoding.spec.ts (1)

22-22: LGTM! Consider adding test.step for better reporting.

The test-id based selectors improve test stability. Both test cases would benefit from using test.step to structure the test flow (setup collection, create request, verify code generation), making the generated reports easier to read.

Also applies to: 68-68

tests/environments/import-environment/global-env-import.spec.ts (1)

11-11: LGTM! Recommend adding test.step for better organization.

The test-id based selector is correct. This test has multiple distinct phases (import collection, import environment, verify variables, test requests) that would benefit from test.step blocks for improved readability and better test reports.

tests/collection/open/open-multiple-collections.spec.ts (1)

99-99: Consider replacing with deterministic wait.

While this waitForTimeout was pre-existing, consider replacing it with a more deterministic wait condition to improve test reliability. For example, wait for the error toast element to be visible.

-    // Wait for error toasts to appear
-    await page.waitForTimeout(1000);
+    // Wait for error toasts to appear
+    const invalidCollectionError = page.getByText('The collection is not valid').first();
+    await expect(invalidCollectionError).toBeVisible({ timeout: 5000 });

tests/preferences/default-collection-location/default-collection-location.spec.js (1)

16-16: Consider replacing arbitrary timeouts with deterministic waits.

These waitForTimeout calls (pre-existing) could be replaced with specific condition waits to make tests more reliable and faster. As per coding guidelines, reduce usage of waitForTimeout unless absolutely necessary.

Also applies to: 34-34, 54-54, 72-72, 92-92

tests/collection/moving-tabs/move-tabs.spec.ts (1)

39-99: Consider reducing waitForTimeout usage.

The test uses multiple page.waitForTimeout() calls that could potentially be replaced with more robust expect() assertions with built-in waits. However, for drag-and-drop operations, some timeout usage may be unavoidable.

Example refactor for lines 39-40:

-    // Wait for the folder to be created and appear in the sidebar
-    await page.waitForTimeout(2000);
-    await expect(page.locator('.collection-item-name').filter({ hasText: 'test-folder' })).toBeVisible();
+    // Wait for the folder to be created and appear in the sidebar
+    await expect(page.locator('.collection-item-name').filter({ hasText: 'test-folder' })).toBeVisible({ timeout: 5000 });

As per coding guidelines, minimizing waitForTimeout usage improves test reliability.

packages/bruno-app/src/components/Sidebar/SidebarHeader/index.js (1)

249-287: Good use of ActionIcon + MenuDropdown for collections header

The refactor to ActionIcon triggers with MenuDropdown (including data-testid prefixes and clear label props for accessibility) is tidy and matches the new menu component’s contract. Keyboard and test selectors should work well with MenuDropdown’s internal behavior.

If you want, you could later extract the header menus into small subcomponents (e.g. CollectionsAddMenu, CollectionsActionsMenu) to keep SidebarHeader lean, but it’s not necessary right now.

packages/bruno-app/src/ui/MenuDropdown/index.js (1)

18-37: JSDoc doesn’t match current behavior for ariaLabel and per-item testId

The docblock still claims:

• ariaLabel “falls back to label or title if not provided”
• each item supports its own testId field

In practice:

• renderMenuItem uses aria-label={item.ariaLabel} and title={item.title} with no fallback
• the data-testid is always derived from the component-level testId plus item.id, and the testId item field is ignored

To avoid confusion for consumers, either:

• implement the documented behavior (fallback label/title and optional per-item testId), or
• simplify the JSDoc to match the actual API (ariaLabel is optional explicit override; test IDs are derived from the top-level data-testid and item.id only).

Given the recent cleanup in callers, updating the docs is probably enough here.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : No trailing commas

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created

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