packages/bruno-app/src/components/Devtools/Console/TerminalTab/index.js (3)

193-216: Empty catch block silently swallows fit errors.

Line 214's empty catch (e) {} hides potential issues. Consider at minimum logging at warn level for debugging, consistent with other error handling in this file.

🔎 Suggested fix

       try {
         instance.fitAddon.fit();
-      } catch (e) {}
+      } catch (e) {
+        console.warn('Error during terminal fit:', e);
+      }

---

391-439: Missing fit in dependency array.

The effect uses fit (lines 403, 405) but doesn't include it in the dependency array. While it works because fit and the effect both depend on activeSessionId, adding fit explicitly satisfies exhaustive-deps and makes the intent clearer.

🔎 Suggested fix

-  }, [activeSessionId]);
+  }, [activeSessionId, fit]);

---

358-388: Consider adding loadSessions to the dependency array.

The polling effect uses loadSessions but has an empty dependency array. While loadSessions is currently stable (memoized with useCallback and empty deps), adding it explicitly would satisfy linter rules and future-proof against changes.

🔎 Suggested fix

-  }, []);
+  }, [loadSessions]);

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.

packages/bruno-app/src/components/Devtools/Console/TerminalTab/index.js (2)

193-216: Consider cleanup for pending RAF on unmount.

The empty catch block at line 214 silently suppresses errors, which might make debugging difficult. Additionally, if the component unmounts while fitRafRef.current holds a pending RAF ID, that frame callback will still execute and attempt to access container and instance, potentially causing issues.

🔎 Add cleanup logic to cancel pending RAF on unmount:

Consider adding a cleanup effect:

+  useEffect(() => {
+    return () => {
+      if (fitRafRef.current) {
+        cancelAnimationFrame(fitRafRef.current);
+        fitRafRef.current = null;
+      }
+    };
+  }, []);

For the empty catch block, consider at least logging the error:

       try {
         instance.fitAddon.fit();
-      } catch (e) {}
+      } catch (e) {
+        console.warn('Failed to fit terminal:', e);
+      }

---

368-384: Verify if triple fitting is necessary.

The terminal undergoes three separate fit operations for a single session activation:

1. Line 176 in openTerminalIntoContainer (after 50ms delay)
2. Line 370 immediately after setup
3. Line 372 after 100ms timeout

While terminal sizing can require layout settling time, three fits may cause unnecessary reflows and performance overhead.

Consider consolidating these fits or verifying whether all three are essential for proper sizing across different scenarios.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

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

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.

packages/bruno-app/src/components/Devtools/Console/TerminalTab/index.js (1)

454-461: CRITICAL: Fix the ResizeObserver cleanup pattern.

This is a duplicate of the previously flagged memory leak concern. The ref callback pattern needs adjustment to ensure proper cleanup. Additionally, as noted by sid-bruno, this can be simplified.

The current implementation creates a new ResizeObserver on every render (because the ref callback is recreated), and while React 19 should call the cleanup function, the early return pattern when node is null may not interact correctly with the cleanup mechanism.

🔎 Apply the suggested fix to ensure proper cleanup:

           <div
             ref={(node) => {
-              if (!node) return;
+              if (!node) {
+                terminalRef.current = null;
+                return;
+              }
               terminalRef.current = node;
               fit();
               const ro = new ResizeObserver(() => fit());
               ro.observe(node.parentNode);
               return () => {
                 ro.disconnect();
               };
             }}
             className="terminal-container"

Based on learnings, React 19 ref callbacks support cleanup functions similar to useEffect, but the pattern must handle the null case explicitly.

packages/bruno-app/src/components/Devtools/Console/TerminalTab/index.js (1)

193-216: Consider documenting or logging the empty catch.

The RAF-debounced fitting logic is sound. However, the empty catch block on line 214 silently suppresses fitAddon.fit() errors. While this may be intentional to prevent UI breakage, consider adding a comment explaining why errors are ignored, or use console.warn for debugging purposes.

🔎 Optional: Add error context

       try {
         instance.fitAddon.fit();
-      } catch (e) {}
+      } catch (e) {
+        // Ignore fit errors during layout transitions
+      }

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

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

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.

packages/bruno-app/src/components/Devtools/Console/TerminalTab/index.js (1)

203-206: Add comment or logging to empty catch block.

The empty catch block silently swallows any errors from fitAddon.fit(). Consider adding a comment explaining why errors are intentionally ignored, or use console.warn for debugging purposes.

🔎 Suggested improvement

     try {
       instance.fitAddon.fit();
-    } catch (e) {}
+    } catch (e) {
+      // Silently ignore fit errors that can occur during rapid layout changes
+    }

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

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

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.

packages/bruno-app/src/components/Devtools/Console/TerminalTab/index.js (1)

401-411: Critical: Stale activeSessionId in ResizeObserver callback.

This issue was previously flagged in past reviews and remains unresolved. When activeSessionId changes (user switches sessions), the ref callback doesn't re-execute because the DOM node doesn't change. The ResizeObserver continues calling fitTerminal with the old session ID, breaking resize functionality after session switches—the exact issue this PR aims to fix.

Example:

1. Mount with session 'A' → observer uses 'A'
2. Switch to session 'B' → observer still uses 'A' (ref callback doesn't fire)
3. Resize devtools → fitTerminal('A', node) called instead of fitTerminal('B', node)

🔎 Recommended fix using a ref to track current session

Store activeSessionId in a ref and read from it in the observer callback:

 const TerminalTab = () => {
   const terminalRef = useRef(null);
+  const activeSessionIdRef = useRef(null);
   const [sessions, setSessions] = useState([]);
   const [activeSessionId, setActiveSessionId] = useState(null);
   const [isLoading, setIsLoading] = useState(true);
+
+  // Keep ref in sync with state
+  useEffect(() => {
+    activeSessionIdRef.current = activeSessionId;
+  }, [activeSessionId]);

Then update the ref callback to use the ref:

   const onSessionMount = useCallback(
     (node) => {
-      if (!node) return;
+      if (!node) {
+        terminalRef.current = null;
+        return;
+      }
       terminalRef.current = node;
-      fitTerminal(activeSessionId, node);
-      const ro = new ResizeObserver(() => fitTerminal(activeSessionId, node));
+      fitTerminal(activeSessionIdRef.current, node);
+      const ro = new ResizeObserver(() => fitTerminal(activeSessionIdRef.current, node));
       ro.observe(node.parentNode);
       return () => ro.disconnect();
     },
-    [activeSessionId]
+    []
   );

Note: Remove activeSessionId from dependencies since we're using the ref.

Based on past review feedback.

packages/bruno-app/src/components/Devtools/Console/TerminalTab/index.js (1)

186-207: Module-level fitFrameRef could cause issues with multiple component instances.

fitFrameRef is declared at module scope, meaning if multiple TerminalTab components are rendered simultaneously (e.g., in different windows or contexts), they'll share the same frame reference. This could lead to cancelled fit operations affecting the wrong component.

Consider moving fitFrameRef into component state via useRef:

🔎 Proposed refactor to scope fitFrameRef per component

 const TerminalTab = () => {
   const terminalRef = useRef(null);
+  const fitFrameRef = useRef(null);
   const [sessions, setSessions] = useState([]);
   // ...

Then update fitTerminal to accept the ref:

-let fitFrameRef;
-const fitTerminal = (activeSessionId, container) => {
+const fitTerminal = (activeSessionId, container, fitFrameRef) => {
   if (!container) return;
 
   const instance = terminalInstances.get(activeSessionId);
   if (!instance?.fitAddon) return;
 
-  if (fitFrameRef) {
-    cancelAnimationFrame(fitFrameRef);
+  if (fitFrameRef.current) {
+    cancelAnimationFrame(fitFrameRef.current);
   }
 
-  fitFrameRef = requestAnimationFrame(() => {
-    fitFrameRef = null;
+  fitFrameRef.current = requestAnimationFrame(() => {
+    fitFrameRef.current = null;
     // ...
   });
 };

And update all fitTerminal calls to pass the ref.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

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

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.