Skip to content

refactor: give duplicated helpers one home#25

Open
vibhavkatre wants to merge 1 commit into
refactor/dead-code-frontendfrom
refactor/dedupe-shared-helpers
Open

refactor: give duplicated helpers one home#25
vibhavkatre wants to merge 1 commit into
refactor/dead-code-frontendfrom
refactor/dedupe-shared-helpers

Conversation

@vibhavkatre

Copy link
Copy Markdown
Collaborator

Second of three cleanup passes (backlog #9). Stacked on #24 — review that one first; this PR's base is refactor/dead-code-frontend, so the diff shown here is only the dedupe work.

Seven helpers existed in two or three copies each. Every copy was byte-identical or trivially equivalent, so each is now defined once and imported.

Helper Copies New home Note
distanceToSegment 3 diagram/geometry.js byte-identical copies in whiteboardModel, strokeSimplify, useFlowchartInteraction
clone 3 utils/clone.js useDiagramStore, history, useClipboard
isEditingText 2 utils/dom.js useKeyboard, useCanvasPaste
wrapLineCount 2 diagram/textMetrics.js same algorithm, two signatures
pointsToPath 2 diagram/svgPath.js useThumbnail imported both under aliases
localStorage JSON read/write 3 utils/localStore.js useRecentColors, useRecentShapes, useAppSettings
minimap navigator (~50 lines) 2 composables/useMinimapNavigator.js Minimap.vue, WhiteboardMinimap.vue

Why these particular merges matter

  • distanceToSegment — every "how far is the cursor from this polyline?" test (stroke simplification, eraser/line hit-testing, flowchart edge picking) now measures through the same function instead of three copies that could drift.
  • clone — the JSON round-trip semantics are now written down. Dropping undefined keys is what keeps a history snapshot equal to what a save persists, so this must not be casually "upgraded" to structuredClone (which keeps them, and throws on the functions a reactive proxy can carry).
  • wrapLineCount — mind-map and flowchart node sizing agreed by coincidence before; now by construction. The shared version takes perLine, and the mind map derives that via the new charsPerLine().
  • minimap navigator — the two navigators duplicated fit-to-box scaling, the viewport rect and click/drag panning, so they could disagree about the same gesture. They differ in exactly one respect, which the composable now models explicitly: Minimap crops the viewport rect to its box (clampedViewRect), WhiteboardMinimap does not (viewRect). Also drops a vestigial element ref in WhiteboardMinimap that nothing read.

One intentional behaviour fix

useAppSettings wrote settings with a bare localStorage.setItem while the other two call sites guarded theirs. In Safari private mode setItem throws, so toggling dark mode threw inside a watcher there. writeJson guards uniformly.

Testing

  • 127 vitest pass (102 before — 25 new tests across textMetrics, svgPath, geometry, clone, dom, localStore)
  • New tests deliberately cover the merged edge cases: long-word wrapping, the "finish the current line before a long word" branch, segment-distance clamping past endpoints, and the localStorage corrupt-JSON / quota / private-mode failure paths
  • yarn build clean

🤖 Generated with Claude Code

Seven helpers existed in two or three copies each. Every copy was byte-identical
or trivially equivalent, so each is now defined once and imported. Consolidation
only — no behaviour change intended, and the equivalence of each merge was
checked against both original implementations.

- distanceToSegment (x3: whiteboardModel, strokeSimplify, useFlowchartInteraction)
  -> diagram/geometry.js. Byte-identical copies. Every "how far is the cursor from
  this polyline?" test — stroke simplification, eraser/line hit-testing, flowchart
  edge picking — now measures through the same function.

- clone (x3: useDiagramStore, history, useClipboard) -> utils/clone.js, with the
  JSON round-trip semantics written down: dropping `undefined` keys is what keeps
  a history snapshot equal to what a save persists, so this must not be casually
  "upgraded" to structuredClone.

- isEditingText (x2: useKeyboard, useCanvasPaste) -> utils/dom.js. Both global
  handlers must bow out on the same surfaces or a shortcut hijacks text entry.

- wrapLineCount (x2: mindmapLayout, flowchartModel) -> diagram/textMetrics.js.
  Same algorithm behind two signatures; the shared one takes `perLine` and the
  mind map now derives that via the new charsPerLine(). Mind-map and flowchart
  node sizing agreed by coincidence before; now by construction.

- pointsToPath (x2: sketch, flowchartLayout) -> diagram/svgPath.js. useThumbnail
  imported BOTH under aliases, so the duplication was already known. The kept
  version is the general one (optional Z close); flowchart routes produce the
  same string as before.

- localStorage JSON read/write (x3: useRecentColors, useRecentShapes,
  useAppSettings) -> utils/localStore.js.

- The minimap navigator (Minimap.vue + WhiteboardMinimap.vue) ->
  composables/useMinimapNavigator.js. ~50 lines of fit-to-box scaling, viewport
  rect and click/drag panning were duplicated; the two navigators could disagree
  about the same gesture. They differ in exactly one respect, which the composable
  now models explicitly: Minimap crops the viewport rect to its box
  (clampedViewRect), WhiteboardMinimap does not (viewRect). Also drops a
  vestigial element ref in WhiteboardMinimap that nothing read.

One intentional behaviour fix falls out of the localStorage consolidation:
useAppSettings wrote settings with a bare localStorage.setItem, while the other
two guarded theirs. In Safari private mode setItem throws, so toggling dark mode
threw inside a watcher there. writeJson guards uniformly.

Tests: 127 vitest pass (102 before; 25 new across textMetrics, svgPath, geometry,
clone, dom and localStore, covering the merged edge cases — long-word wrapping,
segment-distance clamping, and the localStorage failure paths). Build clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants