packages/bruno-app/src/components/Preferences/StyledWrapper.js (1)

40-45: Fixed viewport sizing may cause issues on smaller screens.

The 70vh min/max height and 50vw max-width could clip content or cause poor UX on smaller displays. Consider using responsive breakpoints or more flexible constraints.

packages/bruno-app/src/components/Modal/StyledWrapper.js (2)

104-114: Redundant border-radius declarations.

Line 109 sets border-radius: 0px; but line 113 immediately overrides it with ${(props) => props.theme.border.radius.sm}. Remove the first declaration.

Proposed fix

     .textbox {
       line-height: 1.42857143;
       border: 1px solid #ccc;
       padding: 0.45rem;
       box-shadow: none;
-      border-radius: 0px;
       outline: none;
       box-shadow: none;
       transition: border-color ease-in-out 0.1s;
       border-radius: ${(props) => props.theme.border.radius.sm};

---

104-121: Duplicate box-shadow: none; declarations.

Lines 108 and 111 both declare box-shadow: none;. Remove the duplicate.

packages/bruno-app/src/components/Sidebar/Collections/Collection/RemoveCollection/StyledWrapper.js (1)

4-8: Use theme token for border-radius consistency.

Other StyledWrapper files in this PR use ${(props) => props.theme.border.radius.sm} or .base for border-radius. Hardcoding 4px here breaks consistency.

Proposed fix

   .collection-info-card {
     background-color: ${(props) => props.theme.modal.title.bg};
-    border-radius: 4px;
+    border-radius: ${(props) => props.theme.border.radius.sm};
     padding: 12px;
   }

packages/bruno-app/src/components/Preferences/Display/Font/index.js (1)

41-46: Debounced function recreated on every handleSave change — consider useMemo or stable ref.

Since handleSave depends on preferences, each save triggers a new handleSave → new debouncedSave. This cancels the previous debounce and may cause unexpected behavior during rapid changes. Consider using useMemo with an empty dependency array and referencing handleSave via a ref, or use useRef to store a stable debounced function.

Alternative pattern using ref

+ const handleSaveRef = useRef(handleSave);
+ useEffect(() => {
+   handleSaveRef.current = handleSave;
+ }, [handleSave]);
+
+ const debouncedSave = useMemo(
+   () => debounce((font, fontSize) => {
+     handleSaveRef.current(font, fontSize);
+   }, 500),
+   []
+ );
- const debouncedSave = useCallback(
-   debounce((font, fontSize) => {
-     handleSave(font, fontSize);
-   }, 500),
-   [handleSave]
- );

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

91-92: Empty catch block silently swallows validation errors.

Consider logging the error or providing user feedback when validation fails, similar to the pattern in General/index.js.

Proposed fix

       .catch((error) => {
+        console.error('Proxy validation error:', error.message);
       });

---

78-100: proxySchema in dependency array causes unnecessary re-renders.

proxySchema is defined inside the component and creates a new reference on each render. Move it outside the component or memoize it to stabilize the dependency.

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

136-137: Empty catch block silently swallows validation errors.

Same issue as ProxySettings — consider logging the error for debugging purposes.

Proposed fix

         .catch((error) => {
+          console.error('Preferences validation error:', error.message);
         });

---

130-140: Same debounce recreation pattern — consider extracting to a custom hook.

All preference components (Font, ProxySettings, General, Beta) use the same debounced auto-save pattern. Consider extracting this into a reusable hook like useDebouncedSave to reduce duplication and centralize the fix for the dependency recreation issue.

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

70-80: betaSchema in dependency array is recreated each render.

generateValidationSchema() is called on line 45 every render, producing a new schema reference. This causes debouncedSave to be recreated constantly. Memoize the schema:

Proposed fix

- const betaSchema = generateValidationSchema();
+ const betaSchema = useMemo(() => generateValidationSchema(), []);

And add useMemo to the imports on line 1.

---

76-77: Empty catch block silently swallows validation errors.

Consistent with other preference components — consider logging for debugging.

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

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.

Learnt from: NikHillAgar
Repo: usebruno/bruno PR: 6279
File: packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js:79-86
Timestamp: 2025-12-02T20:48:05.670Z
Learning: In React 19+, ref callbacks support returning cleanup functions. When a ref callback returns a function, React will call it when the element unmounts or the ref changes, similar to useEffect cleanup. This is documented at https://react.dev/learn/manipulating-the-dom-with-refs#how-to-manage-a-list-of-refs-using-a-ref-callback

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

27-34: Replace fixed timeouts with locator-based waiting.

The test now relies on page.waitForTimeout() for both auto-save completion and modal close verification. Per coding guidelines, fixed timeouts should be avoided when locator-based waiting is possible.

🔎 Suggested improvements

-    // wait for auto-save to complete (debounce is 500ms)
-    await page.waitForTimeout(1000);
-
-    // close the preferences
-    await page.locator('[data-test-id="modal-close-button"]').click();
-
-    // wait for modal to close
-    await page.waitForTimeout(500);
+    // wait for auto-save to complete (debounce is 500ms)
+    await page.waitForTimeout(1000);
+
+    // close the preferences
+    const modalCloseButton = page.locator('[data-test-id="modal-close-button"]');
+    await modalCloseButton.click();
+
+    // wait for modal to close
+    const preferencesModal = page.locator('.bruno-modal-card').filter({ hasText: 'Preferences' });
+    await expect(preferencesModal).not.toBeVisible();

This replaces the 500ms fixed timeout with an assertion that the modal is no longer visible, making the test more reliable and faster when the modal closes quickly.

---

47-54: Apply the same locator-based waiting pattern.

Same issue as lines 27-34. The fixed 500ms timeout should be replaced with a locator-based assertion.

🔎 Suggested improvement

-    // wait for auto-save to complete (debounce is 500ms)
-    await page.waitForTimeout(1000);
-
-    // close the preferences
-    await page.locator('[data-test-id="modal-close-button"]').click();
-
-    // wait for modal to close
-    await page.waitForTimeout(500);
+    // wait for auto-save to complete (debounce is 500ms)
+    await page.waitForTimeout(1000);
+
+    // close the preferences
+    const modalCloseButton = page.locator('[data-test-id="modal-close-button"]');
+    await modalCloseButton.click();
+
+    // wait for modal to close
+    const preferencesModal = page.locator('.bruno-modal-card').filter({ hasText: 'Preferences' });
+    await expect(preferencesModal).not.toBeVisible();

tests/onboarding/sample-collection.spec.ts (1)

96-97: Consider wrapping modal interaction in test.step.

The modal confirmation flow would benefit from being wrapped in a test.step for better report readability, per coding guidelines.

🔎 Suggested improvement

-    // Confirm removal in the modal
-    await page.locator('[data-testid="close-collection-modal-title"]', { hasText: 'Close Collection' }).waitFor({ state: 'visible' });
-    await page.locator('.bruno-modal-footer .submit').click();
+    await test.step('Confirm removal in the modal', async () => {
+      await page.locator('[data-testid="close-collection-modal-title"]', { hasText: 'Close Collection' }).waitFor({ state: 'visible' });
+      await page.locator('.bruno-modal-footer .submit').click();
+    });

This improves report readability by clearly separating the modal interaction as a distinct step.

tests/preferences/autosave/autosave.spec.ts (1)

46-51: Add assertion after modal close for reliability.

While the explicit modal close is good, adding an assertion that the modal is no longer visible would make the test more robust and align with Playwright best practices.

🔎 Suggested improvement

     // Wait for auto-save to complete (debounce is 500ms)
     await page.waitForTimeout(1000);
 
     // Close preferences modal
     await preferencesModal.locator('[data-testid="modal-close-button"]').click();
-    await expect(preferencesModal).not.toBeVisible();
+    
+    // Verify modal is closed
+    await expect(preferencesModal).not.toBeVisible();

Apply the same pattern to lines 96-101 and 173-178 for consistency.

tests/utils/page/actions.ts (2)

20-24: Use locator variables for better readability.

Per coding guidelines, locator variables should be used instead of repeating the same locator selector multiple times.

🔎 Suggested improvement

       await firstCollection.locator('.collection-actions .icon').click();
       await page.locator('.dropdown-item').getByText('Remove').click();
-      // Wait for the close collection modal to be visible
-      await page.locator('[data-testid="close-collection-modal-title"]', { hasText: 'Close Collection' }).waitFor({ state: 'visible' });
+      
+      // Wait for the close collection modal to be visible
+      const closeCollectionModal = page.locator('[data-testid="close-collection-modal-title"]', { hasText: 'Close Collection' });
+      await closeCollectionModal.waitFor({ state: 'visible' });
       await page.locator('.bruno-modal-footer .submit').click();
-      // Wait for the close collection modal to be hidden
-      await page.locator('[data-testid="close-collection-modal-title"]', { hasText: 'Close Collection' }).waitFor({ state: 'hidden' });
+      
+      // Wait for the close collection modal to be hidden
+      await closeCollectionModal.waitFor({ state: 'hidden' });

This makes the code more maintainable and follows the coding guidelines for using locator variables.

---

291-293: Apply the same locator variable pattern.

Same recommendation as lines 20-24. Use a locator variable instead of repeating the selector.

🔎 Suggested improvement

     await locators.dropdown.item('Remove').click();
 
-    // Wait for and confirm modal
-    await page.locator('[data-testid="close-collection-modal-title"]', { hasText: 'Close Collection' }).waitFor({ state: 'visible' });
+    // Wait for and confirm modal
+    const closeCollectionModal = page.locator('[data-testid="close-collection-modal-title"]', { hasText: 'Close Collection' });
+    await closeCollectionModal.waitFor({ state: 'visible' });
     await locators.modal.button('Remove').click();
-    await page.locator('[data-testid="close-collection-modal-title"]', { hasText: 'Close Collection' }).waitFor({ state: 'hidden' });
+    await closeCollectionModal.waitFor({ state: 'hidden' });

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

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.

Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:23.647Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.

tests/import/file-types/file-input-acceptance.spec.ts (1)

19-19: Extract locator to a variable per coding guidelines.

The coding guidelines specify to use locator variables for locators. For consistency with line 9 and better readability, store the modal close button locator in a variable.

🔎 Suggested refactor

    // Cleanup: close any open modals
-   await page.getByTestId('modal-close-button').click();
+   const modalCloseButton = page.getByTestId('modal-close-button');
+   await modalCloseButton.click();

Based on coding guidelines: "Use locator variables for locators"

tests/preferences/support-links.spec.js (1)

3-27: Consider using test.step to improve test reporting.

The coding guidelines emphasize using test.step to make generated reports easier to read. Consider wrapping logical sections of this test in steps:

🔎 Suggested refactor with test.step

 test('Should verify all support links with correct URL in preference > Support tab', async ({ page }) => {
-  // Open Preferences
-  await page.getByLabel('Open Preferences').click();
-
-  // Go to Support tab
-  await page.getByRole('tab', { name: 'Support' }).click();
-
-  // Verify all support links with correct URL
-  const locator_twitter = page.getByRole('link', { name: 'Twitter' });
-  expect(await locator_twitter.getAttribute('href')).toEqual('https://twitter.com/use_bruno');
-
-  const locator_github = page.getByRole('link', { name: 'GitHub', exact: true });
-  expect(await locator_github.getAttribute('href')).toEqual('https://github.com/usebruno/bruno');
-
-  const locator_discord = page.getByRole('link', { name: 'Discord', exact: true });
-  expect(await locator_discord.getAttribute('href')).toEqual('https://discord.com/invite/KgcZUncpjq');
-
-  const locator_reportissues = page.getByRole('link', { name: 'Report Issues', exact: true });
-  expect(await locator_reportissues.getAttribute('href')).toEqual('https://github.com/usebruno/bruno/issues');
-
-  const locator_documentation = page.getByRole('link', { name: 'Documentation', exact: true });
-  expect(await locator_documentation.getAttribute('href')).toEqual('https://docs.usebruno.com');
-
-  await page.getByTestId('modal-close-button').click();
+  await test.step('Open Preferences and navigate to Support tab', async () => {
+    await page.getByLabel('Open Preferences').click();
+    await page.getByRole('tab', { name: 'Support' }).click();
+  });
+
+  await test.step('Verify all support links with correct URLs', async () => {
+    const locator_twitter = page.getByRole('link', { name: 'Twitter' });
+    expect(await locator_twitter.getAttribute('href')).toEqual('https://twitter.com/use_bruno');
+
+    const locator_github = page.getByRole('link', { name: 'GitHub', exact: true });
+    expect(await locator_github.getAttribute('href')).toEqual('https://github.com/usebruno/bruno');
+
+    const locator_discord = page.getByRole('link', { name: 'Discord', exact: true });
+    expect(await locator_discord.getAttribute('href')).toEqual('https://discord.com/invite/KgcZUncpjq');
+
+    const locator_reportissues = page.getByRole('link', { name: 'Report Issues', exact: true });
+    expect(await locator_reportissues.getAttribute('href')).toEqual('https://github.com/usebruno/bruno/issues');
+
+    const locator_documentation = page.getByRole('link', { name: 'Documentation', exact: true });
+    expect(await locator_documentation.getAttribute('href')).toEqual('https://docs.usebruno.com');
+  });
+
+  await test.step('Close preferences modal', async () => {
+    await page.getByTestId('modal-close-button').click();
+  });
 });

As per coding guidelines.

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

12-13: Consider clarifying or removing the TODO comment.

The TODO mentions removing data-test-id and using data-testid, but this component already uses only data-testid (line 13). The TODO may refer to a broader codebase cleanup, but placing it here might be confusing.

Either:

• Update the TODO to reflect broader scope (e.g., "TODO: Remove remaining data-test-id usage across other components")
• Remove it if the migration is complete for this component
• Move it to a tracking issue

tests/import/postman/invalid-missing-info.spec.ts (1)

23-23: LGTM! Improved selector strategy.

The migration to getByTestId('modal-close-button') is more reliable than the previous selector approach and follows Playwright best practices.

Optional: Consider extracting locator and using test.step

Per coding guidelines, you could improve test readability:

+    const modalCloseButton = page.getByTestId('modal-close-button');
+
     // Cleanup: close any open modals
-    await page.getByTestId('modal-close-button').click();
+    await test.step('Cleanup: close modal', async () => {
+      await modalCloseButton.click();
+    });

This makes reports easier to read and follows the locator variable pattern.

tests/import/insomnia/malformed-structure.spec.ts (1)

23-23: LGTM! Improved selector strategy.

The migration to getByTestId('modal-close-button') is more reliable than the previous selector approach and follows Playwright best practices.

Optional: Consider extracting locator and using test.step

Per coding guidelines, you could improve test readability:

+    const modalCloseButton = page.getByTestId('modal-close-button');
+
     // Cleanup: close any open modals
-    await page.getByTestId('modal-close-button').click();
+    await test.step('Cleanup: close modal', async () => {
+      await modalCloseButton.click();
+    });

This makes reports easier to read and follows the locator variable pattern.

tests/import/openapi/malformed-yaml.spec.ts (1)

25-25: LGTM! Improved selector strategy.

The migration to getByTestId('modal-close-button') is more reliable than the previous selector approach and follows Playwright best practices.

Optional: Consider extracting locator and using test.step

Per coding guidelines, you could improve test readability:

+    const modalCloseButton = page.getByTestId('modal-close-button');
+
     // Cleanup: close any open modals
-    await page.getByTestId('modal-close-button').click();
+    await test.step('Cleanup: close modal', async () => {
+      await modalCloseButton.click();
+    });

This makes reports easier to read and follows the locator variable pattern.

packages/bruno-app/src/components/Sidebar/Collections/Collection/RemoveCollection/index.js (1)

37-39: Consider simplifying the null collection guard.

The current guard returns a <div> that would render within a modal, but onConfirm already handles the missing collection case with a toast. You could simplify this to return null; or rely solely on the onConfirm guard since the modal is unlikely to be rendered without a valid collection.

🔎 Suggested simplification

  if (!collection) {
-   return <div>Collection not found</div>;
+   return null;
  }

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

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.

Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:23.647Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.

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