# DiagramFlow — Technical Review

**Scope reviewed:** `index.html` (108 lines), `app.js` (839 lines), `styles.css` (378 lines)
**Date:** current session
**Reviewer note:** This is a single-file-per-concern vanilla JS app with no build step. That's a good fit for the project's size, but the single `app.js` IIFE has grown into a monolith that mixes state, rendering, geometry math, and DOM event wiring in one scope. The plan below is organized so it can be done incrementally without a rewrite.

---

## 1. Bugs Found

### 1.1 Bring to Front / Send to Back does nothing (real bug)
`toFront()` / `toBack()` reorder the **array** `S.shapes`, but paint order in the DOM is controlled by `el.style.zIndex = s.zIndex`, which is a **static value set once** at creation (`zIndex: S.shapes.length + 1`) and never updated by `toFront`/`toBack`.

- Location: `app.js` lines ~120–126 (`toFront`/`toBack`) vs line 97 (`zIndex` assignment) vs line 177 (`el.style.zIndex = s.zIndex`).
- Effect: the context menu items "Bring to Front" / "Send to Back" visibly do nothing, because moving a shape's position in the array doesn't change its fixed `zIndex` number.
- Fix: either (a) drop `zIndex` entirely and rely purely on DOM/array order (simplest — SVG/DOM painting is already in array order), or (b) make `toFront`/`toBack` recompute `zIndex` for **all** shapes based on new array order. Recommend (a) — remove the property and the inline `z-index` style; let `renderShapes()` append in array order, which is already correct.

### 1.2 Dead/no-op event listener
```js
connsLayer.addEventListener('mousedown', function(e) { /* comment explaining it never fires */ });
```
This handler is registered but can never fire because `#connections-layer` has `pointer-events: none` in CSS. It's dead code that documents an abandoned approach — should be deleted, not left in with an apologetic comment.

### 1.3 Connection hit-test duplicated (and can drift)
The exact same "walk all connections, compute `distToSegment`, keep closest within threshold" logic is written out fully **twice**: once in `mousedown` (line ~388) and again in `contextmenu` (line ~535). Any future change (e.g., adjusting the hit threshold, or supporting curved connectors) has to be made in two places and can silently diverge. This isn't a bug yet, but it's a latent one.

### 1.4 `conn-hit` SVG paths are unused dead weight
`renderConns()` emits a `<path class="conn-hit" data-cid="...">` for every connection, styled with `pointer-events: stroke` — but the parent `#connections-layer` has `pointer-events: none`, which **disables pointer-events on all descendants regardless of their own value**. So:
- The `.conn-hit:hover` CSS rule never triggers.
- The `data-cid` attribute is never read by any code (`grep` confirms no `data-cid` reads in `app.js`).
- This is pure dead markup being generated on every render, doubling the path elements per connection for no benefit.

Fix: delete the `conn-hit` path and its CSS. Selection already works via the geometric `distToSegment` check in JS.

### 1.5 Locked shapes can still be dragged
`s.locked` disables resize handles (`if (S.selected.includes(s.id) && !s.locked)`), but the **drag** branch has no locked check:
```js
if (hit) {
    selectShape(hit.id, e.shiftKey);
    S.isDragging = true; // no `!hit.locked` guard
    ...
}
```
A "locked" shape can still be moved by dragging — only resizing is actually blocked. This contradicts user expectations of what "lock" means.

### 1.6 Text tool always creates a `rect`
`S.tool === 'text'` hardcodes `addShape('rect', ...)`. There's no way to add free-floating text without an implicit rectangle container (which also visibly nothing since fill defaults to whatever the toolbar currently holds — it will render with a visible border/fill unless the user has fill=transparent). This is more a missing feature than a bug, but it's worth flagging next to the bugs since it surprises users who expect the "Text" tool to produce plain text.

### 1.7 `readStyleControls()` mutates shared global defaults as a side effect of placement
`readStyleControls()` is called right before `addShape(...)` in two separate call sites (text tool creation, shape button placement) purely to refresh the module-level `defaults` object from the DOM. This works, but it's an easy-to-miss implicit dependency — if a third shape-creation path is added later (e.g., paste, programmatic import) and the call is forgotten, new shapes will silently use stale defaults. This should be replaced by reading the inputs directly inside `addShape`, removing the two-step "sync then create" dance.

### 1.8 No defensive check for `localStorage` quota / corrupted JSON beyond a bare `catch(e) {}`
`init()` wraps `JSON.parse(saved)` in try/catch but silently swallows all errors and falls through to... nothing (no demo data is loaded if parsing fails after `saved` was truthy). Result: a corrupted `localStorage` entry produces a blank canvas with no error and no demo fallback, which will look like the app is "broken" with no clue why.

### 1.9 Minor: duplicate `s.locked` check block appears twice in the file at two different line ranges doing the exact same "add resize handles if selected & unlocked" — this is because `renderShapes()` has one and there's a stray leftover reference/comment in a second place. Confirm during cleanup that there isn't a genuine second render path (there shouldn't be, given Round 1 cleanup removed the "fast render" duplicate, but re-verify no drift crept back in).

---

## 2. Architectural / Readability Issues

### 2.1 Single 839-line IIFE with no internal module boundaries
Everything — state, geometry math, SVG string-building, DOM rendering, event wiring, undo/redo, localStorage — lives in one `(function() { ... })()`. This is fine for ~200 lines; at 839 lines it's the single biggest maintainability risk in the project. There are no natural seams (no comments-as-modules consistently followed, no separation between "pure functions" and "DOM-touching functions").

**Recommendation:** Split into clearly-scoped sections using either:
- Multiple `<script>` files loaded in order (still no build step, zero tooling risk), each attaching to one shared `DF` namespace object, e.g.:
  - `state.js` — the `S` object, `defaults`, undo/redo stack management
  - `geometry.js` — `distToSegment`, `snap`, `clamp`, `toCanvas`, `connPath`, `arrowPoints`, `shapeSVG`
  - `render.js` — `renderShapes`, `renderConns`, `renderGrid`, `renderProps`, `applyTransform`
  - `selection.js` — `selectShape`, `selectConn`, `deselectAll`
  - `events.js` — all `addEventListener` wiring
  - `main.js` — `init()` and boot
- OR, if keeping a single file is preferred for simplicity of deployment, at minimum group with clear `// ============ SECTION ============` banners (already partially done) and enforce that pure geometry/helper functions never touch `S` or the DOM directly — right now some helpers are pure (`distToSegment`, `clamp`) and others silently reach into module state (fine for a small app, but worth being consistent).

### 2.2 Shape and connection selection state should be unified
`S.selected` (shape ids) and `S.selectedConns` (connection ids) are two parallel arrays with almost-duplicate logic: `selectShape`/`selectConn`, `deselectAll` clearing both, `renderProps` branching on which one is populated, delete key removing from both, context menu branching on which one has entries. This is a classic case where a single concept — "the current selection" — has been implemented as two independent state slots that must be kept in sync by hand at every call site.

**Recommendation:** Model selection as one array of `{ kind: 'shape' | 'conn', id }` tuples, or simpler — since shape ids are prefixed `s...` and connection ids `c...` — a single `S.selection` array of ids, with a helper `resolveSelection()` that looks up the right collection by prefix. This collapses ~6 duplicated branches into one, and makes multi-kind selection (e.g., selecting a shape AND a connection at once) trivial to support later if desired, or trivially forbidden by a single check in one place instead of two.

### 2.3 Rendering via `innerHTML` string concatenation
`renderShapes()` (DOM-based) and `renderConns()`/`renderGrid()` (string-concatenated `innerHTML`) use two different rendering styles inconsistently. String concatenation with values like fill colors interpolated directly into SVG attribute strings works today because inputs come from `<input type="color">` (safe, browser-sanitized values) — but the pattern is fragile: if `text` or any future free-text field is ever interpolated into `innerHTML` without escaping, this becomes an XSS/HTML-injection vector. Right now shape `text` is set via `textContent` (safe), but connection styling and the grid pattern use raw string building.

**Recommendation:** Standardize on one rendering approach. Given the app already builds shape DOM nodes with `document.createElement`, do the same for connections rather than `innerHTML` string building — it removes a class of injection risk entirely and unifies the mental model ("we always build DOM nodes, never HTML strings").

### 2.4 Full re-render on every mutation
`render()` calls `renderShapes()` + `renderConns()` + `renderProps()`, and `renderShapes()`/`renderConns()` both do `layer.innerHTML = ''` then rebuild **every** shape/connection from scratch on **every** mousemove during drag/resize. For the current demo size (6 shapes, 6 connections) this is invisible, but it means drag/resize performance scales linearly with diagram size, and will visibly stutter past a few dozen shapes because the whole SVG/DOM subtree is torn down and rebuilt 60 times/sec while dragging.

**Recommendation for future scale:** During an active drag/resize, only update the `style.left/top/width/height` of the dragged element(s) directly (already have the DOM ref via `el`), and only reflow connections whose `from`/`to` touches a moving shape. Defer the full `render()` call to `mouseup`. This is the single highest-value perf/architecture change if diagrams are expected to grow beyond ~20-30 shapes.

### 2.5 Magic numbers scattered through hit-testing and geometry
Values like `10 / S.zoom` (connection hit threshold), `8` (resize handle grab radius), `5` (handle half-size), `14` (conn-hit stroke width, now dead per §1.4), `0.35` (arrowhead spread angle), `30` (duplicate offset) are inlined at their use sites with no named constant. Fine individually, but there's no single place to tune "how forgiving is hit-testing" — a reasonable UX tuning pass currently requires grepping the file for numbers.

**Recommendation:** Hoist these into a `CONFIG` object near the top (alongside `defaults`), e.g. `CONFIG.connHitPx = 10`, `CONFIG.handleGrabPx = 8`, `CONFIG.handleSizePx = 5`, `CONFIG.dupOffset = 30`, `CONFIG.arrowSpread = 0.35`, `CONFIG.arrowSize = 10`.

### 2.6 Property panel DOM lookups mix cached refs and repeated `getElementById`
Most panel inputs are cached once at top of file (`propFill`, `propStroke`, etc.), but `renderProps()` calls `document.getElementById('prop-x')` etc. fresh on every render instead of using cached refs — inconsistent with the caching pattern used everywhere else, and a needless repeated DOM lookup on every state change.

**Recommendation:** Cache `propX`, `propY`, `propWidth`, `propHeight`, `propText` at the top like the other panel inputs, for consistency and minor perf.

### 2.7 No single source of truth for "what shape types exist"
`shapeSVG()`'s `switch` statement is the only place that enumerates valid shape types, but shape buttons in `index.html` (`data-shape="rect"`, etc.) are a second, independent source of truth that must be kept in sync by hand. Adding a new shape today requires touching 3 places (HTML button, `shapeSVG` switch case, and implicitly the default width/height on the button's `data-width`/`data-height`). This is exactly the kind of duplication that caused the "dead shape types" cleanup needed in Round 1 (hexagon/parallelogram left in code after being removed from UI, or vice versa).

**Recommendation:** Define one `SHAPE_DEFS` table in JS (id, label, default w/h, SVG-renderer function, small icon path) and generate the toolbar buttons from it at init time, rather than hand-authoring both the HTML and the switch statement. This makes "add a new shape" a one-object-literal change instead of a two-file, two-place edit.

### 2.8 `README.md` is now inaccurate
The README still documents features removed in Round 1 simplification: hand tool, orthogonal/curved connectors, minimap, export/import, hexagon/parallelogram shapes, drag-and-drop from a sidebar palette, and a keyboard shortcut table referencing keys (`H`, `R`, `Q`, `C`, `E`, `D`, `O`, `X`) that no longer exist in `app.js` (`grep` confirms only `v/l/a/t/g/s/+/-` plus ctrl/cmd combos are wired). This is actively misleading to anyone onboarding onto the project.

**Recommendation:** Rewrite `README.md` to match current reality (small task, see plan below) — this should be done in the same pass as any other cleanup since it's low-risk, high-value.

---

## 3. Concrete Simplification Plan (ordered by effort/impact)

1. **Delete dead code** (~15 min, zero risk):
   - Remove the no-op `connsLayer.addEventListener('mousedown', ...)` block.
   - Remove `conn-hit` path generation in `renderConns()` and its CSS (`.conn-hit`, `.conn-hit:hover`).
   - Remove the unused `zIndex` property and inline `el.style.zIndex` (see fix in §3.2 below — do together).

2. **Fix Bring to Front / Send to Back** (~15 min): remove `zIndex` field entirely; rely on `S.shapes` array order (already correct paint order since `renderShapes` appends in array order). Delete `el.style.zIndex = s.zIndex` line.

3. **Fix locked-shape drag bug** (~5 min): add `&& !hit.locked` guard before starting `S.isDragging`.

4. **Unify connection hit-testing into one function** (~20 min): extract the "find closest connection under point within threshold" logic (currently duplicated in `mousedown` and `contextmenu`) into a single `findConnAt(pos)` helper; call it from both places.

5. **Unify selection model** (~1-2 hrs, moderate risk — touches many call sites): collapse `S.selected` + `S.selectedConns` into one `S.selection` array with id-prefix-based type resolution. Update `renderProps`, delete-key handler, context menu handler, undo/redo reset accordingly. This is the highest-value structural change for long-term maintainability.

6. **Hoist magic numbers into a `CONFIG` object** (~20 min, zero risk).

7. **Cache remaining panel DOM refs** (`prop-x/y/width/height/text`) at top of file (~10 min, zero risk).

8. **Rewrite `README.md`** to reflect current tool set, shortcuts, and file structure (~20 min).

9. **(Larger, optional) Split `app.js` into topic files** under a shared namespace, per §2.1 — do this only after 1–8 are done and stable, since it's a mechanical but large diff.

10. **(Larger, optional) Table-driven shape definitions** (§2.7) — nice-to-have once the above stabilizes; not urgent since shape count is small and static today.

11. **(Larger, optional) Drag/resize render performance**: only fix if/when diagrams commonly exceed ~30 shapes; premature otherwise.

---

## 4. New Functionality Ideas

Roughly ordered by how well they fit the existing architecture (cheap to add first):

1. **Font size control in the properties panel.** `s.fontSize` already exists in the shape model and is rendered (`t.style.fontSize`), but there is no UI to change it — every shape is stuck at 14px. Trivial add: one `<input type="number" id="prop-fontsize">` plus one event handler, mirroring the existing `prop-*` pattern.

2. **Multi-shape style editing feedback.** Currently `applyStyleToSelected` silently no-ops if `S.selected.length !== 1`... actually it operates on the whole array already — but `renderProps()` only shows the panel for exactly one selection (`S.selected.length === 1`). Multi-select + drag works, but multi-select + "change fill for all selected at once" has no UI, since the panel hides for 2+ selections. Worth deciding intentionally: either show the panel for multi-select (applying to all) or explicitly document that style edits require single-selection.

3. **Editable connection endpoints (reroute an existing arrow to a different shape).** Currently connections are permanently bound to two anchor "invisible circle" shapes created at draw time; there's no way to grab an existing arrow and drag its end onto a different shape. This is the most commonly requested feature in diagram tools and fits naturally on top of the existing anchor-shape model — dragging a connection's endpoint could just reassign `conn.from`/`conn.to` to whichever shape is under the cursor at drop.

4. **Snap-to-shape / magnetic connection points.** Right now connections always go center-to-center. Adding directional anchor points (N/S/E/W of each shape) would make generated flowcharts look meaningfully more professional, and is a natural extension of the existing `connPath`/`arrowPoints` functions (just changing which x/y they compute from).

5. **Plain text tool (no bounding shape).** Per bug §1.6 — make text tool create a special `type: 'text'` shape with fully transparent default fill/stroke/width and no resize border shown, rather than a generic `rect` with the currently-selected toolbar colors.

6. **Export/Import as JSON or PNG.** Removed in Round 1 for simplification, but "save your work to a file" (vs. only `localStorage`, which is single-browser/single-machine) is a reasonable feature to bring back later behind a minimal button — especially export-to-PNG/SVG for sharing diagrams outside the app.

7. **Line/arrow style variants** (dashed, dotted) — the connection properties panel already exists (per this session's work); adding a `stroke-dasharray` dropdown is a small, natural extension of `syncStyleControlsToConn`/`renderConns`.

8. **Grid size control.** `S.gridSize` is fixed at `20` with no UI; exposing it (e.g., a small number input near the grid/snap toggle buttons) is cheap and useful for users working at different diagram scales.

9. **Keyboard-driven nudge (arrow keys move selected shape by 1px / 10px with Shift).** Common diagram-editor affordance, cheap to add alongside existing keydown handler, doesn't conflict with any current bindings.

10. **Basic accessibility pass.** No ARIA roles/labels anywhere in `index.html`; toolbar buttons rely solely on `title` tooltips. Even a lightweight pass (aria-label on icon-only buttons, `role="toolbar"`) would meaningfully improve usability for screen-reader users without any architectural change.

---

## 5. Summary

The codebase is in good shape structurally for its size (no framework, no build step, everything traceable) but has accumulated some **duplication debt** across the last two feature rounds (parallel shape/connection selection state, duplicated hit-testing logic, dead markup from an abandoned DOM-hover approach) and one **real functional bug** (Bring to Front/Send to Back is a no-op). None of the issues found are severe — the app works — but the fixes in §3 items 1–7 are all low-risk, high-clarity wins that should be done before adding more features, since several of the "new functionality" ideas in §4 (especially editable connection endpoints and magnetic anchor points) will be considerably easier to implement cleanly once the selection model is unified (§2.2) and connection hit-testing is de-duplicated (§2.4/§3.4).