app/src/features/apiClient/screens/apiClient/components/views/graphql/GraphQLClientView.tsx (1)

506-511: Use the localStorage wrapper and extract the storage key constant.

The code directly accesses window.localStorage instead of using the existing wrapper at app/src/utils/localStorage.ts. Additionally, the storage key "sheet_placement" is a magic string duplicated across multiple files.

Consider these improvements:

1. Use the localStorage wrapper for consistency with the codebase (it handles JSON serialization and key namespacing via getStorageKey)
2. Extract the storage key to a shared constant (e.g., in app/src/componentsV2/BottomSheet/constants.ts or the context file)
3. Add error handling since localStorage can throw in incognito mode or when quota is exceeded

+import { localStorage as localStorageWrapper } from "utils/localStorage";
+
 useLayoutEffect(() => {
   if (isDefaultPlacementRef.current) {
     return;
   }
 
-  const savedPlacement = localStorage.getItem("sheet_placement");
+  try {
+    const savedPlacement = localStorageWrapper.getItem("sheet_placement");
-  if (!savedPlacement) {
+    if (!savedPlacement) {
       isDefaultPlacementRef.current = true;
       const bottomSheetPlacement = window.innerWidth <= 1280 ? BottomSheetPlacement.BOTTOM : BottomSheetPlacement.RIGHT;
       toggleSheetPlacement(bottomSheetPlacement);
+    }
+  } catch (error) {
+    // Fallback to default behavior if localStorage is unavailable
+    isDefaultPlacementRef.current = true;
+    const bottomSheetPlacement = window.innerWidth <= 1280 ? BottomSheetPlacement.BOTTOM : BottomSheetPlacement.RIGHT;
+    toggleSheetPlacement(bottomSheetPlacement);
   }
 }, [toggleSheetPlacement]);

app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx (1)

259-264: Use the localStorage wrapper and verify breakpoint consistency.

Similar to GraphQLClientView, this code directly accesses window.localStorage instead of the wrapper, and uses a magic string for the storage key.

Additionally, this view uses a 1440px breakpoint while GraphQLClientView uses 1280px. This inconsistency may be intentional (different layouts), but should be verified.

Apply similar improvements as suggested for GraphQLClientView:

+import { localStorage as localStorageWrapper } from "utils/localStorage";
+
 useLayoutEffect(() => {
   if (isDefaultPlacementRef.current) {
     return;
   }
-  const savedPlacement = localStorage.getItem("sheet_placement");
+  try {
+    const savedPlacement = localStorageWrapper.getItem("sheet_placement");
-  if (!savedPlacement) {
+    if (!savedPlacement) {
       isDefaultPlacementRef.current = true;
       const bottomSheetPlacement = window.innerWidth < 1440 ? BottomSheetPlacement.BOTTOM : BottomSheetPlacement.RIGHT;
       toggleSheetPlacement(bottomSheetPlacement);
+    }
+  } catch (error) {
+    isDefaultPlacementRef.current = true;
+    const bottomSheetPlacement = window.innerWidth < 1440 ? BottomSheetPlacement.BOTTOM : BottomSheetPlacement.RIGHT;
+    toggleSheetPlacement(bottomSheetPlacement);
   }
 }, [toggleSheetPlacement]);

Verification needed: Confirm whether the different breakpoints (1440px here vs 1280px in GraphQL) are intentional or should be aligned.

app/src/componentsV2/BottomSheet/context/index.tsx (4)

9-14: Use the localStorage wrapper for consistency.

The getStoredPlacement helper directly accesses window.localStorage, while the codebase has a centralized wrapper at app/src/utils/localStorage.ts that handles JSON serialization and key namespacing.

+import { localStorage as localStorageWrapper } from "utils/localStorage";
+
 const SHEET_PLACEMENT_STORAGE_KEY = "sheet_placement";
 
 const getStoredPlacement = (): BottomSheetPlacement | null => {
-  const storedValue = localStorage.getItem(SHEET_PLACEMENT_STORAGE_KEY);
+  try {
+    const storedValue = localStorageWrapper.getItem(SHEET_PLACEMENT_STORAGE_KEY) as string | null;
-  return storedValue === BottomSheetPlacement.BOTTOM || storedValue === BottomSheetPlacement.RIGHT ? storedValue : null;
+    return storedValue === BottomSheetPlacement.BOTTOM || storedValue === BottomSheetPlacement.RIGHT ? storedValue : null;
+  } catch (error) {
+    return null;
+  }
 };

---

41-44: Add error handling for localStorage operations.

The persistPlacement function could throw if localStorage is unavailable (incognito mode) or quota is exceeded. Consider adding error handling to gracefully degrade.

+import { localStorage as localStorageWrapper } from "utils/localStorage";
+
 const persistPlacement = (placement: BottomSheetPlacement) => {
   setSheetPlacement(placement);
-  localStorage.setItem(SHEET_PLACEMENT_STORAGE_KEY, placement);
+  try {
+    localStorageWrapper.setItem(SHEET_PLACEMENT_STORAGE_KEY, placement);
+  } catch (error) {
+    // Silently fail if localStorage is unavailable
+    console.warn("Failed to persist sheet placement:", error);
+  }
 };

---

46-52: Consider removing the redundant synchronization effect.

This effect syncs sheetPlacement with stored/default values on mount and when dependencies change. However, since the state is already initialized with the correct value (lines 36-39), this effect appears redundant.

Additionally, including sheetPlacement in the dependency array could cause unnecessary re-renders or logic conflicts when the user toggles placement.

If the intent is to handle external changes to defaultPlacement (e.g., from props), consider:

1. Removing this effect entirely if defaultPlacement is static
2. Or, if dynamic, remove sheetPlacement from deps and only react to defaultPlacement changes:

 useEffect(() => {
   const storedPlacement = getStoredPlacement();
-  const nextPlacement = storedPlacement ?? defaultPlacement;
-  if (sheetPlacement !== nextPlacement) {
-    setSheetPlacement(nextPlacement);
-  }
- }, [defaultPlacement, sheetPlacement]);
+  if (!storedPlacement) {
+    setSheetPlacement(defaultPlacement);
+  }
+}, [defaultPlacement]);

---

68-87: Use the localStorage wrapper consistently.

The toggle logic directly calls localStorage.setItem (line 77) instead of using the wrapper or the persistPlacement helper defined above.

 const toggleSheetPlacement = useCallback((placement?: BottomSheetPlacement) => {
   if (placement) {
     persistPlacement(placement);
     return;
   }
   setSheetPlacement((prev) => {
     const nextPlacement =
       prev === BottomSheetPlacement.BOTTOM ? BottomSheetPlacement.RIGHT : BottomSheetPlacement.BOTTOM;
 
-    localStorage.setItem(SHEET_PLACEMENT_STORAGE_KEY, nextPlacement);
+    try {
+      localStorageWrapper.setItem(SHEET_PLACEMENT_STORAGE_KEY, nextPlacement);
+    } catch (error) {
+      console.warn("Failed to persist sheet placement:", error);
+    }
 
     if (nextPlacement === BottomSheetPlacement.RIGHT) {
       trackViewBottomSheetOnRightClicked();
     } else {
       trackViewBottomSheetOnBottomClicked();
     }
 
     return nextPlacement;
   });
 }, []);

Alternatively, refactor to use persistPlacement consistently:

 const toggleSheetPlacement = useCallback((placement?: BottomSheetPlacement) => {
   if (placement) {
     persistPlacement(placement);
     return;
   }
-  setSheetPlacement((prev) => {
+  
+  setSheetPlacement((prev) => {
     const nextPlacement =
       prev === BottomSheetPlacement.BOTTOM ? BottomSheetPlacement.RIGHT : BottomSheetPlacement.BOTTOM;
-
-    localStorage.setItem(SHEET_PLACEMENT_STORAGE_KEY, nextPlacement);
-
+    
+    // Track analytics
     if (nextPlacement === BottomSheetPlacement.RIGHT) {
       trackViewBottomSheetOnRightClicked();
     } else {
       trackViewBottomSheetOnBottomClicked();
     }
-
+    
+    // Persist after tracking
+    try {
+      localStorageWrapper.setItem(SHEET_PLACEMENT_STORAGE_KEY, nextPlacement);
+    } catch (error) {
+      console.warn("Failed to persist sheet placement:", error);
+    }
+    
     return nextPlacement;
   });
 }, []);

app/src/features/sessionBook/components/SessionViewerBottomSheet/SessionViewerBottomSheet.tsx (1)

31-34: Use the localStorage wrapper and add error handling.

Similar to the other files, this code directly accesses window.localStorage with a magic string and no error handling.

+import { localStorage as localStorageWrapper } from "utils/localStorage";
+
 useEffect(() => {
-  const savedPlacement = localStorage.getItem("sheet_placement");
+  try {
+    const savedPlacement = localStorageWrapper.getItem("sheet_placement");
-  if (bottomSheetBottomBreakpoint && location.pathname.includes(PATHS.SESSIONS.INDEX) && !savedPlacement) {
+    if (bottomSheetBottomBreakpoint && location.pathname.includes(PATHS.SESSIONS.INDEX) && !savedPlacement) {
       toggleSheetPlacement(BottomSheetPlacement.BOTTOM);
+    }
+  } catch (error) {
+    // Fallback: apply default behavior if localStorage fails
+    if (bottomSheetBottomBreakpoint && location.pathname.includes(PATHS.SESSIONS.INDEX)) {
+      toggleSheetPlacement(BottomSheetPlacement.BOTTOM);
     }
+  }
 }, [bottomSheetBottomBreakpoint, toggleSheetPlacement, location.pathname]);

Additionally, consider extracting "sheet_placement" to the shared constant defined in the BottomSheet context file.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: nafees87n
Repo: requestly/requestly PR: 3655
File: app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunResultView/RunResultContainer/RunResultContainer.tsx:426-426
Timestamp: 2025-10-10T11:08:23.369Z
Learning: In the file `app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunResultView/RunResultContainer/RunResultContainer.tsx`, the `destroyInactiveTabPane={true}` setting is intentional. The loss of state (expandedKeys, scroll position) when switching tabs is acceptable for now and will be addressed in future work.

Learnt from: mustafa-sayyed
Repo: requestly/requestly PR: 3723
File: app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx:146-151
Timestamp: 2025-10-22T22:57:45.959Z
Learning: In the API Client Scripts tab (app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx), the dot indicator should only appear when scripts are present (count > 0). When no scripts exist, the dot should not be rendered. This is the intended behavior per issue #3731.

app/src/componentsV2/BottomSheet/context/index.tsx (2)

44-47: Consider wrapping persistPlacement in useCallback.

While not causing issues currently (since it's only called from the already-memoized toggleSheetPlacement), wrapping persistPlacement in useCallback would be more consistent and future-proof.

Apply this diff:

-  const persistPlacement = (placement: BottomSheetPlacement) => {
+  const persistPlacement = useCallback((placement: BottomSheetPlacement) => {
     setSheetPlacement(placement);
     localStorage.setItem("sheet_placement", placement);
-  };
+  }, [setSheetPlacement]);

---

9-12: Optional: Consider a custom hook to reduce duplication.

The pattern of initializing placement state from storage is repeated across 6+ components:

const [sheetPlacement, setSheetPlacement] = useState(() => {
  const savedPlacement = getStoredPlacement();
  return savedPlacement ?? BottomSheetPlacement.RIGHT;
});

You could extract this into a custom hook:

export const usePersistedSheetPlacement = (defaultPlacement: BottomSheetPlacement) => {
  return useState(() => getStoredPlacement() ?? defaultPlacement);
};

Then consumers would use:

const [sheetPlacement, setSheetPlacement] = usePersistedSheetPlacement(BottomSheetPlacement.RIGHT);

Also applies to: 26-32, 36-41, 44-47, 63-82

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: nafees87n
Repo: requestly/requestly PR: 3655
File: app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunResultView/RunResultContainer/RunResultContainer.tsx:426-426
Timestamp: 2025-10-10T11:08:23.369Z
Learning: In the file `app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunResultView/RunResultContainer/RunResultContainer.tsx`, the `destroyInactiveTabPane={true}` setting is intentional. The loss of state (expandedKeys, scroll position) when switching tabs is acceptable for now and will be addressed in future work.

Learnt from: nafees87n
Repo: requestly/requestly PR: 3417
File: app/src/hooks/workspaceFetcher/useWorkspaceFetcher.ts:4-7
Timestamp: 2025-08-20T11:23:01.896Z
Learning: In useFetchTeamWorkspaces hook, activeWorkspaceId should not be included in the effect dependency array to avoid re-subscription on every workspace change, as per user's preference.

app/src/componentsV2/BottomSheet/context/index.tsx (1)

9-12: Consider using the shared localStorage helper and guarding for non-DOM environments.

getStoredPlacement and persistPlacement access the global localStorage directly; this is fine in the browser, but it bypasses the existing app/src/utils/localStorage helper (which centralizes key prefixing/JSON handling) and will throw if this code ever runs where window.localStorage is unavailable (SSR, some tests). If you expect this context to ever be used outside a pure browser runtime, consider either:

• Switching to the shared localStorage helper, or
• Adding a small guard (e.g. if (typeof window === "undefined" || !window.localStorage) return null;) inside getStoredPlacement / persistPlacement.

Also applies to: 43-46

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: nafees87n
Repo: requestly/requestly PR: 3417
File: app/src/hooks/workspaceFetcher/useWorkspaceFetcher.ts:4-7
Timestamp: 2025-08-20T11:23:01.896Z
Learning: In useFetchTeamWorkspaces hook, activeWorkspaceId should not be included in the effect dependency array to avoid re-subscription on every workspace change, as per user's preference.

app/src/componentsV2/BottomSheet/context/index.tsx (2)

9-12: Consider using the shared localStorage wrapper for consistency

getStoredPlacement and persistPlacement currently call the global localStorage directly with the raw "sheet_placement" key and plain string values. Given you already have a namespaced, JSON-aware storage helper in app/src/utils/localStorage.ts, it may be worth routing these reads/writes through that utility for consistency (and to avoid key collisions), updating both the setter and getStoredPlacement accordingly. If this helper is also used in non-DOM contexts, wrapping access in a small try/catch or environment check would make getStoredPlacement safer to reuse elsewhere.

Also applies to: 42-45

---

62-81: Tighten useCallback dependencies or simplify toggle logic

toggleSheetPlacement is wrapped in useCallback with an empty dependency array while closing over persistPlacement and setSheetPlacement. Because the setter is stable and the logic is side‑effectful but simple, you could either:

• Add the appropriate dependencies (e.g., persistPlacement) to satisfy hook expectations, or
• Drop useCallback entirely and let React recreate this function per render, which is usually fine for a small context like this.

Functionality is correct as written; this is just about hook hygiene and future‑proofing.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: nafees87n
Repo: requestly/requestly PR: 3417
File: app/src/hooks/workspaceFetcher/useWorkspaceFetcher.ts:4-7
Timestamp: 2025-08-20T11:23:01.896Z
Learning: In useFetchTeamWorkspaces hook, activeWorkspaceId should not be included in the effect dependency array to avoid re-subscription on every workspace change, as per user's preference.

Learnt from: rohanmathur91
Repo: requestly/requestly PR: 4000
File: app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts:6-14
Timestamp: 2025-12-11T06:43:01.328Z
Learning: In app/src/features/apiClient/slices/multiWorkspaceView/helpers/ApiClientContextRegistry/hooks.ts, the useApiClientFeatureContext hook uses useMemo without reactivity intentionally. Contexts are expected to be registered before component mount, and reactivity to registry updates is not needed.