Add sidebar ide style

[MohamedAli1937]

Description

Add an IDE-style collapsible sidebar for the Blocks panel.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

How Has This Been Tested?

Describe the tests you ran to verify your changes.

• Existing tests pass
• New tests added
• Manual testing

Screenshots (if applicable)

aa.mov

Checklist

• My code follows the project's style guidelines
• I have performed a self-review
• I have added/updated documentation as needed
• My changes generate no new warnings
• Tests pass locally

[areebahmeddd]

can you link the issue for this pull request?

[MohamedAli1937]

Hi @areebahmeddd,
This PR wasn’t directly linked to an existing issue. I noticed a relevant issue while using the app.

[areebahmeddd]

ah, I believe the right way is to have issue linked for all pr's. helps understand the context and validate the issue 🙂

[MohamedAli1937]

I created an issue for this feature: #114

[ivantha]

◎ Argus Review

This PR implements a well-engineered IDE-style collapsible sidebar and resizable preview pane, featuring localStorage persistence, RAF-throttled resize events, scroll-preserving Blockly resizing, and keyboard shortcuts. The core architecture is sound and includes several nice UX details. However, several issues need attention: tooltip keyboard shortcut strings hardcode "Ctrl+" instead of using the existing mod helper (breaking Mac compatibility), the sidebar resize calculation assumes the page has zero left offset (fragile), toggle buttons lack ARIA attributes (accessibility gap), and the double-RAF scheduling in scheduleWidthUpdate can silently drop intermediate resize updates between frames. No security or data-integrity bugs were found.

Findings (17)

🟡 Sidebar resize uses raw e.clientX as width, assumes zero left page offset

Severity: medium | Category: bug | File: imagelab-frontend/src/components/Layout.tsx:105

In the resize callback, e.clientX is used directly as the sidebar width:

const w = Math.max(MIN_SIDEBAR_WIDTH, Math.min(MAX_SIDEBAR_WIDTH, e.clientX));

This works correctly only when the sidebar's left edge is at x = 0 in viewport coordinates. If any ancestor ever adds left padding/margin, or a non-zero left offset is introduced (e.g., a global nav sidebar), the computed width will be wrong by that offset. Contrast this with resizePreview, which correctly computes width as rect.right - e.clientX via getBoundingClientRect() — that approach is robust to offsets.

Suggestion:

Compute the sidebar width relative to the container's left edge:
```ts
const resize = useCallback(
  (e: MouseEvent) => {
    if (!isResizing || !mainRowRef.current) return;
    const rect = mainRowRef.current.getBoundingClientRect();
    const w = Math.max(MIN_SIDEBAR_WIDTH, Math.min(MAX_SIDEBAR_WIDTH, e.clientX - rect.left));
    pendingSidebarWidth.current = w;
    scheduleWidthUpdate();
  },
  [isResizing, scheduleWidthUpdate]
);

---

#### 🟡 Toggle button tooltips hardcode "Ctrl+" instead of using existing `mod` variable
**Severity:** medium | **Category:** bug | **File:** `imagelab-frontend/src/components/Toolbar.tsx:107`

The file already defines `const mod = isMac ? "⌘" : "Ctrl+"` for platform-aware shortcut labels. However, all four new tooltip strings hardcode `"Ctrl+"`:
```tsx
title={isSidebarCollapsed ? "Show Sidebar (Ctrl+B)" : "Hide Sidebar (Ctrl+B)"}
title={isPreviewCollapsed ? "Show Preview (Ctrl+Shift+E)" : "Hide Preview (Ctrl+Shift+E)"}

The collapsed-state restore button in Layout.tsx also has title="Expand Preview (Ctrl+Shift+E)". On macOS, users see "Ctrl+B" in tooltips while the actual shortcut is ⌘B. All pre-existing toolbar buttons presumably already use mod correctly — this is a regression in consistency.

Suggestion:

Use the `mod` variable and add a shift helper:
```tsx
const modShift = isMac ? "⌘⇧" : "Ctrl+Shift+";

title={isSidebarCollapsed ? `Show Sidebar (${mod}B)` : `Hide Sidebar (${mod}B)`}
title={isPreviewCollapsed ? `Show Preview (${modShift}E)` : `Hide Preview (${modShift}E)`}

Apply the same fix to the restore button in Layout.tsx.

---

#### 🟡 Double-RAF in `scheduleWidthUpdate` can silently drop pending width updates
**Severity:** medium | **Category:** bug | **File:** `imagelab-frontend/src/components/Layout.tsx:80`

The function schedules a two-frame RAF chain:
```ts
rafIdRef.current = requestAnimationFrame(() => {
  flushPendingWidths();              // Frame N+1: drains pending values
  rafIdRef.current = requestAnimationFrame(() => {
    svgResizePreservingScroll(workspace); // Frame N+2: resizes Blockly
    rafIdRef.current = null;
  });
});

Between Frame N+1 (outer fires) and Frame N+2 (inner fires), rafIdRef.current is non-null (inner RAF id). Any mousemove events in this window set pendingSidebarWidth.current but skip scheduling. The inner RAF only calls svgResizePreservingScroll — it never calls flushPendingWidths. Those intermediate pending values are silently dropped until the next mouse move after rafIdRef returns to null, causing visible stutter during fast dragging.

Suggestion:

Use a single RAF and call both operations together:
```ts
const scheduleWidthUpdate = useCallback(() => {
  if (rafIdRef.current === null) {
    rafIdRef.current = requestAnimationFrame(() => {
      flushPendingWidths();
      svgResizePreservingScroll(workspace);
      rafIdRef.current = null;
    });
  }
}, [flushPendingWidths, workspace]);

---

#### 🟡 Unconditional separator renders as orphaned element when `onToggleSidebar` is not provided
**Severity:** medium | **Category:** maintainability | **File:** `imagelab-frontend/src/components/Toolbar.tsx:118`

The sidebar toggle button is conditionally rendered, but the separator after it is always rendered:
```tsx
{onToggleSidebar && (
  <button ...>...</button>
)}

<div className={separator} />  {/* always rendered */}

<button onClick={handleNew} ...

When Toolbar is rendered without onToggleSidebar (e.g., in an isolated test, storybook, or a future alternate layout), the separator appears at the leading edge with nothing before it.

Suggestion:

Wrap the separator inside the conditional block:
```tsx
{onToggleSidebar && (
  <>
    <button onClick={onToggleSidebar} className={iconBtn} title={...}>
      {isSidebarCollapsed ? <PanelLeftOpen size={18} /> : <PanelLeftClose size={18} />}
    </button>
    <div className={separator} />
  </>
)}

---

#### 🟡 Toggle buttons lack ARIA accessibility attributes
**Severity:** medium | **Category:** maintainability | **File:** `imagelab-frontend/src/components/Layout.tsx:220`

All four collapse/expand buttons (sidebar toggle in Toolbar, preview toggle in Toolbar, collapsed sidebar restore, collapsed preview restore) use only `title` for labeling. Screen readers do not reliably announce `title` on focus, and it is invisible on mobile. The buttons have no `aria-expanded`, `aria-label`, or `aria-controls` attributes, making them undiscoverable to assistive technology users.

Affected elements:
- Sidebar toggle in `Toolbar.tsx`
- Preview toggle in `Toolbar.tsx`
- The `ChevronRight` restore button in `Layout.tsx`
- The `ChevronLeft` restore button in `Layout.tsx`

**Suggestion:**

Add ARIA attributes to each button and an id/role to the panels:

<button
  onClick={toggleSidebar}
  aria-expanded={!isSidebarCollapsed}
  aria-controls="sidebar-panel"
  aria-label={isSidebarCollapsed ? 'Show Sidebar' : 'Hide Sidebar'}
  ...
>

// On the Sidebar container div:
<div id="sidebar-panel" role="complementary" aria-label="Blocks panel" ...>

---

#### 🔵 RAF not explicitly cancelled on component unmount
**Severity:** low | **Category:** bug | **File:** `imagelab-frontend/src/components/Layout.tsx:64`

`rafIdRef` can hold a live RAF id during a drag operation. There is no cleanup that calls `cancelAnimationFrame(rafIdRef.current)` when the component unmounts. In React 18, stale state setter calls are silently ignored, but the `svgResizePreservingScroll` call inside the inner RAF will still execute, performing unnecessary DOM mutations on an unmounted component. The workspace resize `useEffect` at line ~152 correctly uses `cancelAnimationFrame` in its cleanup, but the drag RAF lifecycle does not.

**Suggestion:**

Add a cleanup effect:

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

---

#### 🔵 `navigator.platform` is deprecated
**Severity:** low | **Category:** maintainability | **File:** `imagelab-frontend/src/components/Toolbar.tsx:18`

`navigator.platform` is deprecated in the Living Standard. The code uses `|| navigator.userAgent` as fallback, but the preferred modern approach is to check `navigator.userAgentData?.platform` (with fallback) or use `navigator.userAgent` exclusively. This line is pre-existing but the PR touches this file, making it a good opportunity to address it.

**Suggestion:**

const isMac =
  typeof navigator !== 'undefined' &&
  /mac/i.test(
    (navigator as { userAgentData?: { platform?: string } }).userAgentData?.platform ??
    navigator.userAgent
  );

---

#### 🔵 `svgResizePreservingScroll` function is defined between import statements
**Severity:** low | **Category:** style | **File:** `imagelab-frontend/src/hooks/useBlocklyWorkspace.ts:3`

The exported utility function is inserted in the middle of the import block:
```ts
import * as Blockly from 'blockly';

export function svgResizePreservingScroll(...) { ... }  // <-- between imports

import '@blockly/field-angle';
import '@blockly/field-colour';

While valid (function declarations are hoisted), this is flagged by import/order ESLint rules, breaks conventional module structure (all imports before exports), and is confusing to readers. It also conflates a utility function with side-effect import sequencing.

Suggestion:

Move the function after all import statements, or extract it to a dedicated `src/utils/blocklyUtils.ts` file:
```ts
// All imports first...
import { useRef, useEffect, useState, useCallback } from 'react';
import * as Blockly from 'blockly';
import '@blockly/field-angle';
// ... rest of imports

// Then exports:
export function svgResizePreservingScroll(workspace: Blockly.WorkspaceSvg | null) {
  if (!workspace) return;
  ...
}

---

#### 🔵 Preview separator uses hardcoded Tailwind classes instead of the `separator` constant
**Severity:** low | **Category:** style | **File:** `imagelab-frontend/src/components/Toolbar.tsx:207`

The sidebar toggle uses `<div className={separator} />` (the pre-existing `separator` constant), but the preview toggle separator is hardcoded:
```tsx
<div className="w-px h-5 bg-gray-300 mx-1 ml-2" />

Apart from inconsistency, the hardcoded version applies both mx-1 and ml-2 simultaneously, resulting in margin-left: calc(0.25rem + 0.5rem) — almost certainly unintentional and visually asymmetric with the margin-right: 0.25rem from mx-1.

Suggestion:

Replace with the existing separator constant:
```tsx
<div className={separator} />

This ensures visual consistency and removes the accidental double-left-margin.

---

#### 🔵 `Ctrl+Shift+E` conflicts with Firefox DevTools Network Monitor
**Severity:** low | **Category:** maintainability | **File:** `imagelab-frontend/src/hooks/useKeyboardShortcuts.ts:49`

`Ctrl+Shift+E` opens the Network Monitor panel in Firefox DevTools. Developers testing this application in Firefox will find that pressing the shortcut opens DevTools rather than toggling the Preview pane, because browser-native shortcuts take priority over `window` keydown listeners. This specifically affects the developer workflow for this project.

Additionally, `Ctrl+B` (sidebar toggle) is the "bold" shortcut in many rich-text editors and could confuse users who have text inputs focused.

**Suggestion:**

Consider less conflicting shortcuts, for example:

• Ctrl+Shift+P for Preview toggle
• Ctrl+\\ for sidebar toggle (used by VS Code)

At minimum, add a comment:

// NOTE: Ctrl+Shift+E conflicts with Firefox DevTools Network Monitor.
// TODO: Consider changing this shortcut.

---

#### 🔵 CSS selectors target Blockly internal class names
**Severity:** low | **Category:** maintainability | **File:** `imagelab-frontend/src/index.css:51`

```css
.imagelab-resizing .blocklyZoom,
.imagelab-resizing .blocklyTrash,
.imagelab-resizing .blocklyFlyout {
  transition: none !important;
}

.blocklyZoom, .blocklyTrash, and .blocklyFlyout are Blockly's internal CSS class names applied to its SVG elements. These are not part of Blockly's public API and may be renamed, removed, or scoped differently in any Blockly update. A Blockly version bump could silently break this optimization with no TypeScript error or test failure.

Suggestion:

Add a version-pinning comment:
```css
/* Suppress Blockly's internal element transitions during panel resize.
 * Class names verified against blockly@X.Y.Z — re-check after upgrades.
 * See: https://github.com/google/blockly/blob/master/core/css.ts */

---

#### 🔵 Touch/pointer events not supported for resize handles
**Severity:** low | **Category:** maintainability | **File:** `imagelab-frontend/src/components/Layout.tsx:134`

The resize dividers attach only `mousemove`/`mouseup` listeners and the `onMouseDown` handler is mouse-only. On touch devices (iPads, touchscreen laptops) or with stylus input, dragging the resize handle has no effect. The overlay capture div also relies on mouse cursor semantics.

This is a UX gap for tablet users accessing a block-programming interface, which is a plausible use case.

**Suggestion:**

Use the Pointer Events API, which unifies mouse, touch, and stylus. Using setPointerCapture also eliminates the need for the global window listeners and the overlay div:

<div
  className={...}
  onPointerDown={(e) => {
    (e.currentTarget as HTMLElement).setPointerCapture(e.pointerId);
    startResizing();
  }}
  onPointerMove={(e) => {
    if (!isResizing) return;
    const w = Math.max(MIN_SIDEBAR_WIDTH, Math.min(MAX_SIDEBAR_WIDTH, e.clientX));
    pendingSidebarWidth.current = w;
    scheduleWidthUpdate();
  }}
  onPointerUp={stopResizing}
/>

---

#### 🔵 `lastSavedWidths` initialized to `{0, 0}` triggers immediate localStorage write on first render
**Severity:** low | **Category:** performance | **File:** `imagelab-frontend/src/components/Layout.tsx:180`

```ts
const lastSavedWidths = useRef({ sidebar: 0, preview: 0 });

On every page load, the persistence effect fires and sees Math.abs(320 - 0) >= 1 as true, writing the default width back to localStorage immediately — even though the value was just read from localStorage moments before. This is a no-op round-trip write on every mount.

Suggestion:

Initialize `lastSavedWidths` with the actual loaded values to suppress the spurious write:
```ts
const lastSavedWidths = useRef({ sidebar: sidebarWidth, preview: previewWidth });

---

#### ℹ️ Redundant `isResizing` guard in `resize` callback
**Severity:** info | **Category:** style | **File:** `imagelab-frontend/src/components/Layout.tsx:105`

Both `resize` and `resizePreview` begin with:
```ts
if (!isResizing) return;

But these callbacks are only attached to window inside a useEffect that checks if (isResizing). The guard is therefore dead code in normal execution — its only function is as a defensive backstop against a race between effect cleanup and a final event. While not harmful, it forces isResizing into the useCallback dependency array, causing a new function reference each time resizing state toggles.

Suggestion:

Either remove the guard (and note the effect already handles this), or keep it with a clarifying comment:
```ts
// Defensive: effect only registers this listener when isResizing=true,
// but guard against race with effect cleanup.
if (!isResizing) return;

---

#### ℹ️ Captured `ws` local variable is checked but not used inside the ResizeObserver closure
**Severity:** info | **Category:** style | **File:** `imagelab-frontend/src/hooks/useBlocklyWorkspace.ts:106`

```ts
const ws = workspaceRef.current;  // captured here
if (!el || !ws) return;           // used only as guard

const ro = new ResizeObserver(() => {
  // Uses workspaceRef.current directly — not the captured ws
  requestAnimationFrame(() => svgResizePreservingScroll(workspaceRef.current));
});

The callback intentionally reads workspaceRef.current (not ws) to always get the latest workspace reference. This is the correct pattern but the discrepancy between the captured variable and the closure variable is confusing without a comment.

Suggestion:

Add a brief comment explaining the intent:
```ts
const ws = workspaceRef.current;
if (!el || !ws) return; // ws is the initial guard only; closure uses ref for freshness

const ro = new ResizeObserver(() => {
  // Read from ref (not captured ws) so we always resize the current workspace.
  requestAnimationFrame(() => svgResizePreservingScroll(workspaceRef.current));
});

---

#### ℹ️ Positive: `svgResizePreservingScroll` is a clean, well-documented abstraction
**Severity:** info | **Category:** style | **File:** `imagelab-frontend/src/hooks/useBlocklyWorkspace.ts:3`

Exporting `svgResizePreservingScroll` from `useBlocklyWorkspace.ts` and reusing it consistently across `Layout.tsx` is good practice. The JSDoc comment clearly explains the purpose, and saving/restoring `scrollX`/`scrollY` before calling `Blockly.svgResize` is a non-obvious but important detail for preventing viewport jump during panel transitions. This is a well-handled UX detail that shows careful attention to Blockly's behavior.

**Suggestion:**

Consider adding a defensive check for disposed workspaces:

if (!workspace || workspace.isDisposed()) return;

---

#### ℹ️ Positive: Overlay capture div correctly maintains resize cursor during fast drag
**Severity:** info | **Category:** style | **File:** `imagelab-frontend/src/components/Layout.tsx:157`

The pattern of rendering a `fixed inset-0 z-[100] cursor-col-resize pointer-events-auto` overlay div while resizing is the correct approach for maintaining the resize cursor when the mouse moves faster than the 1px-wide handle. This prevents flickering cursor changes as the pointer passes over text, buttons, or iframes (such as the Blockly SVG surface), and ensures `mouseup` is captured globally even if released outside the handle. This is a well-known best practice for custom drag/resize UIs and is implemented correctly here.

**Suggestion:**

No change needed. Note that if the implementation is upgraded to use Pointer Events API with setPointerCapture(), this overlay div becomes unnecessary, further simplifying the DOM.

---


*◎ Generated by [Argus](https://github.com) — AI-Powered PR Review*

[MohamedAli1937]

@ivantha Please review my PR with the fixes applied.

[ivantha]

Thanks for the PR! It currently has merge conflicts with main (or is behind). Could you rebase onto the latest main and push the updated branch? Happy to merge once it's auto-rebasable.

[MohamedAli1937]

@ivantha Rebased onto the latest main and force-pushed. The branch should now be up to date and conflict-free 👍